[10] Marlin2D upgrade 0.7.5

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[10] Marlin2D upgrade 0.7.5

Laurent Bourgès
Hi,

I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with its Java2d variant.

Please review this Marlin2D upgrade to Marlin 0.7.5:

JBS: no bug yet for OpenJDK 10
Changes:
- Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills
- Added Marlin Double-precision pipeline
- MarlinFX backports (Dasher, Stroker changes from OpenPisces)
- dead code & few comment removals in Strokers
- fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants

As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?

PS: I forgot to upgrade the copyright headers to 2017, but will do later

Cheers,
Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Jim Graham-5
This is on my slate of things to finish code reviewing today, but I don't have an OpenJDK10 workspace built yet, so it
will take some time to test a sanity check-build unless Phil wants to handle that...?

                        ...jim

On 4/22/17 4:28 AM, Laurent Bourgès wrote:

> Hi,
>
> I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with
> its Java2d variant.
>
> Please review this Marlin2D upgrade to Marlin 0.7.5:
>
> JBS: no bug yet for OpenJDK 10
> webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/
>
> Changes:
> - Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills
> - Added Marlin Double-precision pipeline
> - MarlinFX backports (Dasher, Stroker changes from OpenPisces)
> - dead code & few comment removals in Strokers
> - fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants
>
> As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?
>
> PS: I forgot to upgrade the copyright headers to 2017, but will do later
>
> Cheers,
> Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Philip Race
I have kicked off a test build.

-phil.

On 04/25/2017 01:24 PM, Jim Graham wrote:

> This is on my slate of things to finish code reviewing today, but I
> don't have an OpenJDK10 workspace built yet, so it will take some time
> to test a sanity check-build unless Phil wants to handle that...?
>
>             ...jim
>
> On 4/22/17 4:28 AM, Laurent Bourgès wrote:
>> Hi,
>>
>> I am pleased to have successfully upgraded my machine with OpenJDK10
>> and merged my Marlin 0.7.5 release (dec 2016) with
>> its Java2d variant.
>>
>> Please review this Marlin2D upgrade to Marlin 0.7.5:
>>
>> JBS: no bug yet for OpenJDK 10
>> webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/
>>
>> Changes:
>> - Improved MarlinTileGenerator to efficiently handle asymetric tiles
>> (W x H) with almost full / empty fills
>> - Added Marlin Double-precision pipeline
>> - MarlinFX backports (Dasher, Stroker changes from OpenPisces)
>> - dead code & few comment removals in Strokers
>> - fixed all floating-point number literals to be x.0f or x.0d to
>> simplify the conversion between float & double variants
>>
>> As this patch is huge, do you want me to provide webrevs against
>> Float vs Double pipelines or against OpenJFX10 ?
>>
>> PS: I forgot to upgrade the copyright headers to 2017, but will do later
>>
>> Cheers,
>> Laurent

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Jim Graham-5
In reply to this post by Laurent Bourgès
Hi Laurent,

- I found some fp constants with no "d" or "f" modifier.  I sent a grep output in a separate email...

- Why does FloatMath.java copy the constants from sun.misc rather than refer to them?
   (An alternative is a static import or having local copies that copy by
    source-based reference (foo_bar = Foo.bar) rather than copying by
    human-cut-and-paste)

- Should FloatMath be renamed to FPMath or IEEEMath since it now also handles doubles?

- MarlinCache - What was the reason to eliminate the mean crossings calculations from the decision on whether or not to
use RLE?

- MarlinCache line 548 - why the simpler logic here?

- MarlinProperties, getLog2_XY - 0 to 4 is 1 to 16 subpixel locations in X or Y but you list the subpixel limits as 1 to
256.  Also, it looks like the real limits are 0 to 8 which really does work out to 1 to 256.  So, I think the "4" in the
comment is incorrect and should be "8"?

- MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I could see a
couple of ways of going here:
    - TileSize means height and TileWidth is new which is just odd naming
      In this case, I'd make the default for TileWidth be the value for TileSize
      otherwise setting tilesize used to set both W&H, but now it only sets H?
    - TileSize is legacy and new values are TileWidth and TileHeight
      Both default to TileSize if not specified
In either case, I would think that TileWidth should default to TileSize?

- MarlinProperties - Inc/Dec - constants don't end with d or f.

- MarlinProperties - getFloat() takes doubles for def/min/max?

- MarlinTileGenerator - lines 67,69 - null is the default value for fields

- MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.  Why not have
MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and be able to
manipulate either type...?

- MarlinTileGenerator.TH_AA_ALPHA_00/FF - seem misnamed, really they are low and high thresholds for filling.  Maybe LO
and HI?

- MarlinTileGenerator, line 416,443,508 - Isn't that "skip" instead of "fill"?

- MarlinTileGenerator, line 420,554,687 - Isn't cx >= aax0?  And isn't x1 >= aax0? ???

- MarlinTileGenerator, line 491 - spacing on byte cast

- MarlinTilegenerator, line 655 - "Clear" or "Fill"?

- Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test into the repo
anywhere?

- Renderer, line 1318,1365 - that was fallout from the half-open stuff at 1391, right?

- Stroker, line 112 - "* 8"?  Or perhaps "* 6 + 2" since the endpoints are shared?

- Stroker, line 347,376 - it appears to still use m[off],m[off+1] instead of 0,1...?

- Stroker, line 377 - safecomputeMiter -> safeComputeMiter?  Also, this looks to be some sort of "compute something for
an offset for quads" method, at least in the way it is used, even if it does look similar to the computeMiter method.

- Stroker, line 841 - "winden" => "widen"

- Stroker, lines 839,847,856,866 - there is no p4

I'm assuming that the D*.java files were all generated from the regular files using a sed script?  I skipped those (for
now)...

                                ...jim


On 4/22/17 4:28 AM, Laurent Bourgès wrote:

> Hi,
>
> I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with
> its Java2d variant.
>
> Please review this Marlin2D upgrade to Marlin 0.7.5:
>
> JBS: no bug yet for OpenJDK 10
> webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/
>
> Changes:
> - Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills
> - Added Marlin Double-precision pipeline
> - MarlinFX backports (Dasher, Stroker changes from OpenPisces)
> - dead code & few comment removals in Strokers
> - fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants
>
> As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?
>
> PS: I forgot to upgrade the copyright headers to 2017, but will do later
>
> Cheers,
> Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Laurent Bourgès
My comments below explaining changes:

2017-04-26 1:47 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

- I found some fp constants with no "d" or "f" modifier.  I sent a grep output in a separate email...

    I fixed all true matches except in comments.
 

- Why does FloatMath.java copy the constants from sun.misc rather than refer to them?
  (An alternative is a static import or having local copies that copy by
   source-based reference (foo_bar = Foo.bar) rather than copying by
   human-cut-and-paste)

    This is needed by MarlinFX as sun.misc.FloatConsts is a class inaccessible from javafx.graphics module.
    I prefer copying these constants (source code) to make Marlin2D / MarlinFX consistent and avoid exposing the FloatConsts class to modules.
 

- Should FloatMath be renamed to FPMath or IEEEMath since it now also handles doubles?

    Agreed and I would prefer FPMath, but it can be done later (52 matches)
 

- MarlinCache - What was the reason to eliminate the mean crossings calculations from the decision on whether or not to use RLE?

    I changed my mind as this calculation only disables 'RLE' and block flag optimization early: it is just a shortcut to avoid testing the real number of crossing per pixel row i.e. it only save few tests per row. To compute the mean crossings, I had to accumulate the edge height so it costs 1 add per edge that represents an overhead for lots of edges. Finally I prefer having simplified a bit the code.
 

- MarlinCache line 548 - why the simpler logic here?

    This is related to modified Renderer calls to copyAARow() to better handle half-open intervals:
        before: [maxX + 2] => now: [maxX + 1]
        // +1 because alpha [pix_minX; pix_maxX[
        copyAARow(_alpha, lastY, minX, maxX + 1, useBlkFlags);
 

- MarlinProperties, getLog2_XY - 0 to 4 is 1 to 16 subpixel locations in X or Y but you list the subpixel limits as 1 to 256.  Also, it looks like the real limits are 0 to 8 which really does work out to 1 to 256.  So, I think the "4" in the comment is incorrect and should be "8"?

    Fixed getSubPixel_Log2_X()
 

- MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I could see a couple of ways of going here:
   - TileSize means height and TileWidth is new which is just odd naming
     In this case, I'd make the default for TileWidth be the value for TileSize
     otherwise setting tilesize used to set both W&H, but now it only sets H?
   - TileSize is legacy and new values are TileWidth and TileHeight
     Both default to TileSize if not specified
In either case, I would think that TileWidth should default to TileSize?

    Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value (W=H)
 

- MarlinProperties - Inc/Dec - constants don't end with d or f.

    Fixed.
 

- MarlinProperties - getFloat() takes doubles for def/min/max?

    Fixed.
 

- MarlinTileGenerator - lines 67,69 - null is the default value for fields

    I prefer to be explicit (netbeans reports a warning): left unchanged.
 

- MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.  Why not have MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and be able to manipulate either type...?

    I agree. I tried but benchamrks proved that interface calls method were slower than monomorphic calls so I adopted this bimorphic call optimization where only 1 type is really used at runtime.
 

- MarlinTileGenerator.TH_AA_ALPHA_00/FF - seem misnamed, really they are low and high thresholds for filling.  Maybe LO and HI?

    Renamed as TH_AA_ALPHA_FILL_EMPTY & TH_AA_ALPHA_FILL_FULL
 

- MarlinTileGenerator, line 416,443,508 - Isn't that "skip" instead of "fill"?

    Fixed.
 

- MarlinTileGenerator, line 420,554,687 - Isn't cx >= aax0?  And isn't x1 >= aax0? ???

    Fixed comment:
    // now: cx >= x0 and cx >= aax0
 

- MarlinTileGenerator, line 491 - spacing on byte cast

    Fixed.
 

- MarlinTilegenerator, line 655 - "Clear" or "Fill"?

    Fixed.
 

- Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test into the repo anywhere?

    Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)
 

- Renderer, line 1318,1365 - that was fallout from the half-open stuff at 1391, right?

    Exactly, I fixed handling half-open intervals in MarlinFX so it is a backport.
 

- Stroker, line 112 - "* 8"?  Or perhaps "* 6 + 2" since the endpoints are shared?

    Yes endpoints are shared so I fixed the capacity
 

- Stroker, line 347,376 - it appears to still use m[off],m[off+1] instead of 0,1...?

    Fixed comments.
 

- Stroker, line 377 - safecomputeMiter -> safeComputeMiter?  Also, this looks to be some sort of "compute something for an offset for quads" method, at least in the way it is used, even if it does look similar to the computeMiter method.

    Renamed the method; it was backported from openpisces (so I did not really study the approach)
 

- Stroker, line 841 - "winden" => "widen"

    Fixed.
 

- Stroker, lines 839,847,856,866 - there is no p4

    Already mentioned: see https://bugs.openjdk.java.net/browse/JDK-8170029
    To be fixed later
 

I'm assuming that the D*.java files were all generated from the regular files using a sed script?  I skipped those (for now)...

I also fixed D* files.

Laurent


On 4/22/17 4:28 AM, Laurent Bourgès wrote:
Hi,

I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with
its Java2d variant.

Please review this Marlin2D upgrade to Marlin 0.7.5:

JBS: no bug yet for OpenJDK 10
webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/

Changes:
- Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills
- Added Marlin Double-precision pipeline
- MarlinFX backports (Dasher, Stroker changes from OpenPisces)
- dead code & few comment removals in Strokers
- fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants

As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?

PS: I forgot to upgrade the copyright headers to 2017, but will do later

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Laurent Bourgès
Hi Jim,

Would you have time to look at this 2nd webrev ?

Laurent

Le 26 avr. 2017 11:32 PM, "Laurent Bourgès" <[hidden email]> a écrit :
My comments below explaining changes:

2017-04-26 1:47 GMT+02:00 Jim Graham <[hidden email]>:
Hi Laurent,

- I found some fp constants with no "d" or "f" modifier.  I sent a grep output in a separate email...

    I fixed all true matches except in comments.
 

- Why does FloatMath.java copy the constants from sun.misc rather than refer to them?
  (An alternative is a static import or having local copies that copy by
   source-based reference (foo_bar = Foo.bar) rather than copying by
   human-cut-and-paste)

    This is needed by MarlinFX as sun.misc.FloatConsts is a class inaccessible from javafx.graphics module.
    I prefer copying these constants (source code) to make Marlin2D / MarlinFX consistent and avoid exposing the FloatConsts class to modules.
 

- Should FloatMath be renamed to FPMath or IEEEMath since it now also handles doubles?

    Agreed and I would prefer FPMath, but it can be done later (52 matches)
 

- MarlinCache - What was the reason to eliminate the mean crossings calculations from the decision on whether or not to use RLE?

    I changed my mind as this calculation only disables 'RLE' and block flag optimization early: it is just a shortcut to avoid testing the real number of crossing per pixel row i.e. it only save few tests per row. To compute the mean crossings, I had to accumulate the edge height so it costs 1 add per edge that represents an overhead for lots of edges. Finally I prefer having simplified a bit the code.
 

- MarlinCache line 548 - why the simpler logic here?

    This is related to modified Renderer calls to copyAARow() to better handle half-open intervals:
        before: [maxX + 2] => now: [maxX + 1]
        // +1 because alpha [pix_minX; pix_maxX[
        copyAARow(_alpha, lastY, minX, maxX + 1, useBlkFlags);
 

- MarlinProperties, getLog2_XY - 0 to 4 is 1 to 16 subpixel locations in X or Y but you list the subpixel limits as 1 to 256.  Also, it looks like the real limits are 0 to 8 which really does work out to 1 to 256.  So, I think the "4" in the comment is incorrect and should be "8"?

    Fixed getSubPixel_Log2_X()
 

- MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I could see a couple of ways of going here:
   - TileSize means height and TileWidth is new which is just odd naming
     In this case, I'd make the default for TileWidth be the value for TileSize
     otherwise setting tilesize used to set both W&H, but now it only sets H?
   - TileSize is legacy and new values are TileWidth and TileHeight
     Both default to TileSize if not specified
In either case, I would think that TileWidth should default to TileSize?

    Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value (W=H)
 

- MarlinProperties - Inc/Dec - constants don't end with d or f.

    Fixed.
 

- MarlinProperties - getFloat() takes doubles for def/min/max?

    Fixed.
 

- MarlinTileGenerator - lines 67,69 - null is the default value for fields

    I prefer to be explicit (netbeans reports a warning): left unchanged.
 

- MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.  Why not have MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and be able to manipulate either type...?

    I agree. I tried but benchamrks proved that interface calls method were slower than monomorphic calls so I adopted this bimorphic call optimization where only 1 type is really used at runtime.
 

- MarlinTileGenerator.TH_AA_ALPHA_00/FF - seem misnamed, really they are low and high thresholds for filling.  Maybe LO and HI?

    Renamed as TH_AA_ALPHA_FILL_EMPTY & TH_AA_ALPHA_FILL_FULL
 

- MarlinTileGenerator, line 416,443,508 - Isn't that "skip" instead of "fill"?

    Fixed.
 

- MarlinTileGenerator, line 420,554,687 - Isn't cx >= aax0?  And isn't x1 >= aax0? ???

    Fixed comment:
    // now: cx >= x0 and cx >= aax0
 

- MarlinTileGenerator, line 491 - spacing on byte cast

    Fixed.
 

- MarlinTilegenerator, line 655 - "Clear" or "Fill"?

    Fixed.
 

- Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test into the repo anywhere?

    Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)
 

- Renderer, line 1318,1365 - that was fallout from the half-open stuff at 1391, right?

    Exactly, I fixed handling half-open intervals in MarlinFX so it is a backport.
 

- Stroker, line 112 - "* 8"?  Or perhaps "* 6 + 2" since the endpoints are shared?

    Yes endpoints are shared so I fixed the capacity
 

- Stroker, line 347,376 - it appears to still use m[off],m[off+1] instead of 0,1...?

    Fixed comments.
 

- Stroker, line 377 - safecomputeMiter -> safeComputeMiter?  Also, this looks to be some sort of "compute something for an offset for quads" method, at least in the way it is used, even if it does look similar to the computeMiter method.

    Renamed the method; it was backported from openpisces (so I did not really study the approach)
 

- Stroker, line 841 - "winden" => "widen"

    Fixed.
 

- Stroker, lines 839,847,856,866 - there is no p4

    Already mentioned: see https://bugs.openjdk.java.net/browse/JDK-8170029
    To be fixed later
 

I'm assuming that the D*.java files were all generated from the regular files using a sed script?  I skipped those (for now)...

I also fixed D* files.

Laurent


On 4/22/17 4:28 AM, Laurent Bourgès wrote:
Hi,

I am pleased to have successfully upgraded my machine with OpenJDK10 and merged my Marlin 0.7.5 release (dec 2016) with
its Java2d variant.

Please review this Marlin2D upgrade to Marlin 0.7.5:

JBS: no bug yet for OpenJDK 10
webrev: http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.0/

Changes:
- Improved MarlinTileGenerator to efficiently handle asymetric tiles (W x H) with almost full / empty fills
- Added Marlin Double-precision pipeline
- MarlinFX backports (Dasher, Stroker changes from OpenPisces)
- dead code & few comment removals in Strokers
- fixed all floating-point number literals to be x.0f or x.0d to simplify the conversion between float & double variants

As this patch is huge, do you want me to provide webrevs against Float vs Double pipelines or against OpenJFX10 ?

PS: I forgot to upgrade the copyright headers to 2017, but will do later

Cheers,
Laurent



--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Jim Graham-5
In reply to this post by Laurent Bourgès
Hi Laurent,

A couple of comments that might just be points for clarification.  Minimally it would make sense to include the JBS
report number on the third item below, but the rest are just notes and suggestions...

On 4/26/17 2:32 PM, Laurent Bourgès wrote:

>     - MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I could
>     see a couple of ways of going here:
>        - TileSize means height and TileWidth is new which is just odd naming
>          In this case, I'd make the default for TileWidth be the value for TileSize
>          otherwise setting tilesize used to set both W&H, but now it only sets H?
>        - TileSize is legacy and new values are TileWidth and TileHeight
>          Both default to TileSize if not specified
>     In either case, I would think that TileWidth should default to TileSize?
>
>
>     Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value (W=H)

I was leaning towards adding W & H and having Size be the old mechanism - for symmetry - but this is fine.

>     - MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.  Why not
>     have MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and be able
>     to manipulate either type...?
>
>
>     I agree. I tried but benchamrks proved that interface calls method were slower than monomorphic calls so I adopted
> this bimorphic call optimization where only 1 type is really used at runtime.

I'm curious how much difference this made to require this amount of complexity, but there is a solution here if you are
really worried about performance - use a super-class instead of an interface.  If you can measure overhead for invoking
an abstract method then there is something wrong with the VM.

>     - Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test into the
>     repo anywhere?
>
>
>     Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)

I'd add the JBS number "JDK_NNNNNNNN" as well then.

                                ...jim
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Laurent Bourgès
Jim,


Changes:
- Added 'TestNonAARasterization (JDK-8170879)' in (D)Stroker classes
- Fixed two comments related to half-open intervals in (D)Renderer classes
- Fixed copyright year to 2017
- Double-precision Marlin2D enabled by default in RenderingEngine:

-            final String marlinREClass = "sun.java2d.marlin.MarlinRenderingEngine";
+            final String marlinREClass = "sun.java2d.marlin.DMarlinRenderingEngine";


My comments below:


On 4/26/17 2:32 PM, Laurent Bourgès wrote:
    - MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I could
    see a couple of ways of going here:
       - TileSize means height and TileWidth is new which is just odd naming
         In this case, I'd make the default for TileWidth be the value for TileSize
         otherwise setting tilesize used to set both W&H, but now it only sets H?
       - TileSize is legacy and new values are TileWidth and TileHeight
         Both default to TileSize if not specified
    In either case, I would think that TileWidth should default to TileSize?


    Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value (W=H)

I was leaning towards adding W & H and having Size be the old mechanism - for symmetry - but this is fine.

Agreed but we could add that later when we will increase the tile width & height (asymmetric) for performance. Few adjustments remain in java2d pipeline classes.
 

    - MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.  Why not
    have MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and be able
    to manipulate either type...?


    I agree. I tried but benchamrks proved that interface calls method were slower than monomorphic calls so I adopted
this bimorphic call optimization where only 1 type is really used at runtime.

I'm curious how much difference this made to require this amount of complexity, but there is a solution here if you are really worried about performance - use a super-class instead of an interface.  If you can measure overhead for invoking an abstract method then there is something wrong with the VM.

I tested this issue a lot in december and this 'bimorphic' optimization is concluding. I agree having an abstract class would be good but extracting common parts between Renderer & DRenderer is not easy and it may be more difficult to maintain.
 

    - Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test into the
    repo anywhere?


    Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)

I'd add the JBS number "JDK_NNNNNNNN" as well then.

Fixed.

Cheers,
Laurent
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [10] Marlin2D upgrade 0.7.5

Jim Graham-5
This looks fine, but I've reached out to Phil with a question about changing the default and whether we need to file a
request for that.

Is there a JBS bug for this yet?

                                ...jim

On 5/9/17 3:06 AM, Laurent Bourgès wrote:

> Jim,
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/Marlin-075.2/
>
> Changes:
> - Added 'TestNonAARasterization (JDK-8170879)' in (D)Stroker classes
> - Fixed two comments related to half-open intervals in (D)Renderer classes
> - Fixed copyright year to 2017
> - Double-precision Marlin2D enabled by default in RenderingEngine:
>
> -            final String marlinREClass = "sun.java2d.marlin.MarlinRenderingEngine";
> +            final String marlinREClass = "sun.java2d.marlin.DMarlinRenderingEngine";
>
>
> My comments below:
>
>
>     On 4/26/17 2:32 PM, Laurent Bourgès wrote:
>
>             - MarlinProperties - TileSize vs TileWidth.  Is there a reason you haven't created a TileHeight property?  I
>         could
>             see a couple of ways of going here:
>                - TileSize means height and TileWidth is new which is just odd naming
>                  In this case, I'd make the default for TileWidth be the value for TileSize
>                  otherwise setting tilesize used to set both W&H, but now it only sets H?
>                - TileSize is legacy and new values are TileWidth and TileHeight
>                  Both default to TileSize if not specified
>             In either case, I would think that TileWidth should default to TileSize?
>
>
>             Fixed, I adopted the first solution and getTileWidth_Log2() uses getTileSize_Log2() to get the default value
>         (W=H)
>
>
>     I was leaning towards adding W & H and having Size be the old mechanism - for symmetry - but this is fine.
>
>
> Agreed but we could add that later when we will increase the tile width & height (asymmetric) for performance. Few
> adjustments remain in java2d pipeline classes.
>
>
>
>             - MarlinTileGenerator,MarlinRenderer - all of the methods called on rdrF and rdrD have the same signature.
>         Why not
>             have MarlinRenderer include those methods and then you just need to store a single MarlinRenderer field and
>         be able
>             to manipulate either type...?
>
>
>             I agree. I tried but benchamrks proved that interface calls method were slower than monomorphic calls so I
>         adopted
>         this bimorphic call optimization where only 1 type is really used at runtime.
>
>
>     I'm curious how much difference this made to require this amount of complexity, but there is a solution here if you
>     are really worried about performance - use a super-class instead of an interface.  If you can measure overhead for
>     invoking an abstract method then there is something wrong with the VM.
>
>
> I tested this issue a lot in december and this 'bimorphic' optimization is concluding. I agree having an abstract class
> would be good but extracting common parts between Renderer & DRenderer is not easy and it may be more difficult to
> maintain.
>
>
>
>             - Renderer, line 85,114 - maybe add a line saying that is the result of <name> test?  Did we put that test
>         into the
>             repo anywhere?
>
>
>             Added comment: 'TestNonAARasterization: cubics/quads' (not in repo, only in JBS)
>
>
>     I'd add the JBS number "JDK_NNNNNNNN" as well then.
>
>
> Fixed.
>
> Cheers,
> Laurent
Loading...