[10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Laurent Bourgès
Hi,

Please review this webrev of the Marlin renderer 0.8.2 for JDK-10 (jdk/client forrest). It provides an efficient path clipper in both Stroker (float / double variants) & Filler fixing the bug JDK-8191814
I worked hard on this release since august and several patches were already proposed 0.8.0 (Stroker only clipping) ... and discussed here: http://mail.openjdk.java.net/pipermail/2d-dev/2017-August/008509.html
Build + tests: OK (see the new jtreg ClipShapeTest)

(I hope to provide the same upgrade to OpenJFX10 once this patch is reviewed)

Detailled change log:
JDK-8191814

Marlin 0.8.2 changelog:
- (D)Curve: corrected computation of curve coefficients to minimize error (stable formula)
- (D)Dasher:
    - lots of syntax changes
    - goTo(): use new method goTo_starting() to put slow path in another method (hotspot)
    - lineTo() / somethingTo(): use local variables in loops for higher efficiency
- (D)Helpers:
    - removed imports of Math functions (inlined in cubicRootsInAB)
    - removed unused polyLineLength()
    - added outcode() copied from GeneralRenderer
    - moved PolyStack class from (D)Stroker + use given Stat classes in constructor
    - added new IndexStack class to store 'corner' (integer) indices for the new PathClipFilter (Filler case): see push() that removes redundant corners (direction flip) and pullAll() that returns the corner points from all stored indices
- (D)MarlinRenderingEngine:
    - added new clip settings (doClip / doClipRuntimeFlag)
    - strokeTo(): added trace wrapper (disabled) and detectClosedPath() to know in advance if the path is closed and properly handle caps for filled polylines
    - getAATileGenerator(): define the initial clip bounds for both Stroker / Filler cases + use the new PathClipFilter (Filler case)
- (D)Renderer:
    - added constant for subpixel offsets (used to adjust the effective clipping area)
    - moveTo() / lineTo() / quadTo() / curveTo() : renamed arguments
    - closePath(): skip useless addLine() call if P0 = S0
- (D)RendererContext: added clipping state: doClip & closedPath flags, clipRect area (ymin, ymax, xmin, xmax) + increased array cache capacities for new [Poly/Index]Stack instances

- (D)Stroker:
Approach: ignore outside segments (moveTo) without computing intersections ie it opens the path (polyline) so extra care is needed to properly handle visible cap (and joins).
    - fixed constants (CAP / JOIN) moved into MarlinConst
    - fixed PolyStack usage (moved into (D)Helper)
    - added clipping state (current / starting outcodes, opened / capStart flags)
    - init(): adjust the clipping rectangle with the stroker margin (miter limit, width) and (scaled) renderer offsets (if delta transformation in use)
    - moveTo(): finish() the current opened subpath + compute starting & current outcode
    - lineTo() / quadTo() / curveTo() : added clipping = trivial segment rejection based on point outcodes that 'opens' the path and calls moveTo()
    - closePath(): tricky (but tested) changes to emit moveTo() or lineTo() if the closing segment is visible or not (and joins) + only emit close if the path is inside
    - finish(): if ClosedPathDetector indicated the path is not closed: use the outcode argument (current point) to only emit visible end cap and visible start cap (starting outcode)
    - drawJoin(): added outcode argument (current point) to only emit visible round / miter joins

- (D)TransformingPathConsumer2D:
    - moved all fields at the beginning
    - added ClosedPathDetector, PathClipFilter, PathTracer instances and wrappers
    - deltaTransformConsumer(): adjust clip rectangle to deal with Stroker coordinate transformations on the clipping area (inverse transform)
    - adjustClipInverseDelta(): for the shear transformation, the clipping area is set to the bounding box (rectangle) of the 4 transformed points of the initial clipping area
    - added the ClosedPathDetector class: simply set rdrCtx.closedPath to true in closePath()
    - added the PathClipFilter class: (Filler case)
        Approach: clipping closed polygons is more complex than Stroker (opened paths) to ignore outside segments but maintain the polygon closed i.e. insert corner points (on the clip boundary) when an outside segment crosses regions (T / L / B / R)
        - init(): adjust the clipping rectangle with the renderer offsets
        - finishPath(): totally ignore the path if the accumulated outcode is outside
        - finish(): initialize corner points (if needed) and use the index stack to emit all stored corners before emitting lineTo() to the current point
        - closePath() / pathDone(): use finishPath() to emit remaining points
        - moveTo(): initialize clipping state (outcodes, outside)
        - lineTo() / quadTo() / curveTo() : added clipping = trivial segment rejection based on point outcodes and calls clip() to insert corner points if needed + update the accumulated outcode (for finishPath)
        - clip(): tricky (but tested) solution to insert 1 or 2 corner points in the index stack when the segment outcodes are different but only on left or right sides
    - added the PathTracer class that log PathConsumer2D methods (std out)

- MarlinCache: fixed syntax of the useRLE flag (shorter conditions)
- MarlinConst: moved all WIND / CAP / JOIN constants here + added outcode constants (sides & masks)
- MarlinProperties: added system properties for clipping
- RendererStats: updated statistics for new features
- Version: updated version to "marlin-0.8.2-Unsafe-OpenJDK"

- added new ClipShapeTest (jtreg) that checks all possible combinations of (cap / join) for random polyline (Stroker) and polygons (Filler) comparing image outputs rendered with clipping enabled vs disabled

Thanks for your time,

Cheers,
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Sergey Bylokhov
Hi, Laurent.
On 29/11/2017 14:30, Laurent Bourgès wrote:
> - added new ClipShapeTest (jtreg) that checks all possible combinations
> of (cap / join) for random polyline (Stroker) and polygons (Filler)
> comparing image outputs rendered with clipping enabled vs disabled

I have only one note that the test is passed before the fix, so if we
will regress at some point later we will not catch this.


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Laurent Bourgès
Hi Sergey,

Thanks for your comment.

This new test only validates the new clipping algorithms ie it compares the rendering outputs with / without clipping enabled.

As such algorithms are only available in Marlin 0.8.2 and the test uses new system properties to enable/disable clipping, I confirm it passes before (jdk9 or jdk10 before patch).

To ensure it detects any regression, I manually inserted some bugs in the clipping code, and the test failed.

Note: I should add another test @run to check the float variant too (and not only the double variant, the default in jdk10).

Finally I could write a new performance test that would prove clipping is more efficient than before.
Such test would fail before patch (timeout ?), but it is difficult to make it robust as it depends on the hw. 
Jim wrote a basic test in the jfx bug showing 300ms without but 2ms now => gain is high. 
A possible success condition would be: clipping gain > 10 or 50.

Regards,
Laurent


Le 4 déc. 2017 11:11 PM, "Sergey Bylokhov" <[hidden email]> a écrit :
Hi, Laurent.

On 29/11/2017 14:30, Laurent Bourgès wrote:
- added new ClipShapeTest (jtreg) that checks all possible combinations of (cap / join) for random polyline (Stroker) and polygons (Filler) comparing image outputs rendered with clipping enabled vs disabled

I have only one note that the test is passed before the fix, so if we will regress at some point later we will not catch this.


--
Best regards, Sergey.

Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Laurent Bourgès
Hi again,

Here is a new webrev fixing the ClipShapeTest:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8191814.1/

Changes:
- @run twice to test both Marlin variants (float / double)
- use Logger to get & check marlin system properties ("sun.java2d.renderer.clip.runtime.enable") to ensure the test is run against a Marlin renderer having the clipping features

I tested the fixed test on JDK9 and it fails:
runner finished test: sun/java2d/marlin/ClipShapeTest.java
Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Marlin clipping not enabled at runtime !

On JDK10 + patch, it passes:
sun/java2d/marlin/ClipShapeTest.java                                                                                    Passed. Execution successful


PS: Could anyone review the patch ? before RDP1 deadline ?

Regards,
Laurent


2017-12-04 23:35 GMT+01:00 Laurent Bourgès <[hidden email]>:
Hi Sergey,

Thanks for your comment.

This new test only validates the new clipping algorithms ie it compares the rendering outputs with / without clipping enabled.

As such algorithms are only available in Marlin 0.8.2 and the test uses new system properties to enable/disable clipping, I confirm it passes before (jdk9 or jdk10 before patch).

To ensure it detects any regression, I manually inserted some bugs in the clipping code, and the test failed.

Note: I should add another test @run to check the float variant too (and not only the double variant, the default in jdk10).

Finally I could write a new performance test that would prove clipping is more efficient than before.
Such test would fail before patch (timeout ?), but it is difficult to make it robust as it depends on the hw. 
Jim wrote a basic test in the jfx bug showing 300ms without but 2ms now => gain is high. 
A possible success condition would be: clipping gain > 10 or 50.

Regards,
Laurent


Le 4 déc. 2017 11:11 PM, "Sergey Bylokhov" <[hidden email]> a écrit :
Hi, Laurent.

On 29/11/2017 14:30, Laurent Bourgès wrote:
- added new ClipShapeTest (jtreg) that checks all possible combinations of (cap / join) for random polyline (Stroker) and polygons (Filler) comparing image outputs rendered with clipping enabled vs disabled

I have only one note that the test is passed before the fix, so if we will regress at some point later we will not catch this.


--
Best regards, Sergey.




--
--
Laurent Bourgès
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Sergey Bylokhov
I am not an expert here, I just look to the changes and run the closed
tests for jdk_desktop group. No new issues were found, so it looks fine
to me.

On 05/12/2017 06:30, Laurent Bourgès wrote:

> Hi again,
>
> Here is a new webrev fixing the ClipShapeTest:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8191814.1/
>
> Changes:
> - @run twice to test both Marlin variants (float / double)
> - use Logger to get & check marlin system properties
> ("sun.java2d.renderer.clip.runtime.enable") to ensure the test is run
> against a Marlin renderer having the clipping features
>
> I tested the fixed test on JDK9 and it fails:
> runner finished test: sun/java2d/marlin/ClipShapeTest.java
> Failed. Execution failed: `main' threw exception:
> java.lang.RuntimeException: Marlin clipping not enabled at runtime !
>
> On JDK10 + patch, it passes:
> sun/java2d/marlin/ClipShapeTest.java                                                                                    
> Passed. Execution successful
>
>
> PS: Could anyone review the patch ? before RDP1 deadline ?
>
> Regards,
> Laurent
>
>
> 2017-12-04 23:35 GMT+01:00 Laurent Bourgès <[hidden email]
> <mailto:[hidden email]>>:
>
>     Hi Sergey,
>
>     Thanks for your comment.
>
>     This new test only validates the new clipping algorithms ie it
>     compares the rendering outputs with / without clipping enabled.
>
>     As such algorithms are only available in Marlin 0.8.2 and the test
>     uses new system properties to enable/disable clipping, I confirm it
>     passes before (jdk9 or jdk10 before patch).
>
>     To ensure it detects any regression, I manually inserted some bugs
>     in the clipping code, and the test failed.
>
>     Note: I should add another test @run to check the float variant too
>     (and not only the double variant, the default in jdk10).
>
>     Finally I could write a new performance test that would prove
>     clipping is more efficient than before.
>     Such test would fail before patch (timeout ?), but it is difficult
>     to make it robust as it depends on the hw.
>     Jim wrote a basic test in the jfx bug showing 300ms without but 2ms
>     now => gain is high.
>     A possible success condition would be: clipping gain > 10 or 50.
>
>     Regards,
>     Laurent
>
>
>     Le 4 déc. 2017 11:11 PM, "Sergey Bylokhov"
>     <[hidden email] <mailto:[hidden email]>> a
>     écrit :
>
>         Hi, Laurent.
>
>         On 29/11/2017 14:30, Laurent Bourgès wrote:
>
>             - added new ClipShapeTest (jtreg) that checks all possible
>             combinations of (cap / join) for random polyline (Stroker)
>             and polygons (Filler) comparing image outputs rendered with
>             clipping enabled vs disabled
>
>
>         I have only one note that the test is passed before the fix, so
>         if we will regress at some point later we will not catch this.
>
>
>         --
>         Best regards, Sergey.
>
>
>
>
>
> --
> --
> Laurent Bourgès


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: [10] RFR JDK-8191814: Marlin rasterizer spends time computing geometry for stroked segments that do not intersect the clip

Laurent Bourgès
Thanks for your reviews, Phil & Sergey !

Laurent


2017-12-07 20:43 GMT+01:00 Sergey Bylokhov <[hidden email]>:
I am not an expert here, I just look to the changes and run the closed tests for jdk_desktop group. No new issues were found, so it looks fine to me.

On 05/12/2017 06:30, Laurent Bourgès wrote:
Hi again,

Here is a new webrev fixing the ClipShapeTest:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-082-8191814.1/

Changes:
- @run twice to test both Marlin variants (float / double)
- use Logger to get & check marlin system properties ("sun.java2d.renderer.clip.runtime.enable") to ensure the test is run against a Marlin renderer having the clipping features

I tested the fixed test on JDK9 and it fails:
runner finished test: sun/java2d/marlin/ClipShapeTest.java
Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: Marlin clipping not enabled at runtime !

On JDK10 + patch, it passes:
sun/java2d/marlin/ClipShapeTest.java                                                                                    Passed. Execution successful


PS: Could anyone review the patch ? before RDP1 deadline ?

Regards,
Laurent


2017-12-04 23:35 GMT+01:00 Laurent Bourgès <[hidden email] <mailto:[hidden email]>>:

    Hi Sergey,

    Thanks for your comment.

    This new test only validates the new clipping algorithms ie it
    compares the rendering outputs with / without clipping enabled.

    As such algorithms are only available in Marlin 0.8.2 and the test
    uses new system properties to enable/disable clipping, I confirm it
    passes before (jdk9 or jdk10 before patch).

    To ensure it detects any regression, I manually inserted some bugs
    in the clipping code, and the test failed.

    Note: I should add another test @run to check the float variant too
    (and not only the double variant, the default in jdk10).

    Finally I could write a new performance test that would prove
    clipping is more efficient than before.
    Such test would fail before patch (timeout ?), but it is difficult
    to make it robust as it depends on the hw.
    Jim wrote a basic test in the jfx bug showing 300ms without but 2ms
    now => gain is high.
    A possible success condition would be: clipping gain > 10 or 50.

    Regards,
    Laurent


    Le 4 déc. 2017 11:11 PM, "Sergey Bylokhov"
    <[hidden email] <mailto:[hidden email]>> a
    écrit :

        Hi, Laurent.

        On 29/11/2017 14:30, Laurent Bourgès wrote:

            - added new ClipShapeTest (jtreg) that checks all possible
            combinations of (cap / join) for random polyline (Stroker)
            and polygons (Filler) comparing image outputs rendered with
            clipping enabled vs disabled


        I have only one note that the test is passed before the fix, so
        if we will regress at some point later we will not catch this.