[OpenJDK 2D-Dev] RFR 8148886: SEGV in sun.java2d.marlin.Renderer._endRendering

[Jim Graham]

These changes look fine, but my previous comments remain...

			...jim

On 2/8/16 12:58 PM, Laurent Bourgès wrote:
> Jim & Phil,
>
> I updated my previous webrev to have MarlinRenderingEngine TL/CLQ more
> close to the new proposed approach in AAShapePipe (and looks simpler):
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.2/
>
> Please give me your comments on my previous email.
>
> Cheers,
> Laurent
>
>
> 2016-02-06 0:21 GMT+01:00 Laurent Bourgès <bourges.laurent at gmail.com
> <mailto:bourges.laurent at gmail.com>>:
>
>     Jim & Phil,
>
>     Here is an updated webrev according to jim's comments:
>     http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.1/
>
>     Changes:
>     - AAShapePipe rewritten to use a new ReentrantThreadLocal class
>     (generalization) that use TL + CLQ (like Marlin) to use other
>     (cached) TileState instances in case of reentrancy; such class may
>     be useful for other use cases (replacing synchronized / static
>     refs); it do not use WeakReference as TileState instances are very
>     small (to be improved later ?)
>     - Improved CrashPaintTest to check few pixel values to ensure
>     rendering is correct
>
>     PS: I noticed that AlphaPaintPipe has a constant int TILE_SIZE = 32
>     that seems useless as the method has a tilesize argument; maybe it
>     should be cleaned up ?
>
>     Regards,
>     Laurent
>
>     2016-02-05 2:20 GMT+01:00 Jim Graham <james.graham at oracle.com
>     <mailto:james.graham at oracle.com>>:
>
>         Ah, at least one error spotted here after my initial review...
>
>         The abox values could be changed by the code in lines 157-160 so
>         you can't cache their values until after that.
>
>         Also, as you pointed out in another thread in the grdev group,
>         we should probably do the same treatment for the TileState in
>         case we have reentrance for the AAShapePipe code...
>
>                          ...jim
>
>
>         On 2/4/2016 5:10 PM, Jim Graham wrote:
>
>             Hi Laurent,
>
>             In AAShapePipe you load the values from abox[] into
>             variables named
>             xywh, but these are not xywh values, they are min/max
>             values.  We
>             typically use any of the following naming conventions for
>             these types of
>             values:
>
>             - x0, y0, x1, y1
>             - x1, y1, x2, y2
>             - minX, minY, maxX, maxY
>
>             Other than that naming inconsistency the changes look great...
>
>                           ...jim
>
>             On 2/4/2016 2:21 PM, Laurent Bourgès wrote:
>
>                 Please review the webrev fixing SEGFAULT (P2) in the
>                 Marlin Renderer
>                 when using thread-local storage with custom Paint
>                 (reentrance):
>                 bug: https://bugs.openjdk.java.net/browse/JDK-8148886
>                 webrev:
>                 http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.0/
>
>                 Changes:
>                 - detect reentrance in MarlinRenderingEngine using flag
>                 RendererContext.usedTL (true/false) and use another
>                 context from CLQ
>                 - added few more details in array checks (XXXArrayCache)
>                 - fixed AAShapePipe to support reentrancy: added
>                 defensive copy of int[]
>                 abox in local variables + TODO to discuss
>                 - updated Version to 0.7.3.2
>                 - fixed copyright headers
>
>                 Best regards,
>                 Laurent
>
>
>
>
>     --
>     --
>     Laurent Bourgès
>
>
>
>
> --
> --
> Laurent Bourgès