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

[Jim Graham]

You might want to separate the new ReentrantTL class into a separate 
file in sun.java2d so it can be used in other places.  What are the 
hurdles for using it in Marlin instead of rolling its own?  If it is 
going to be reused it might be better to represent the ReentrantContext 
class as an interface so that objects with pre-determined super-class 
hierarchies can still participate.

TILE_SIZE is probably mostly obsolete now, but that's fodder for some 
other work.  Also, 32 may not be the best choice to use when we aren't 
using Ductus, but currently D3D can only accept 32 so there is work to 
be done before that is really configurable.

MarlinRenderingEngine line 182 - testing useThreadLocal is 
redundant/irrelevant here, checking the queue variable should be enough.

ReentrantThreadLocal - depth is a poor name for that field.  Perhaps 
usage?  With "*_TL_INACTIVE", "*_TL_IN_USE", and "*_CLQ" being the variants?

AAShapePipe line 297 - should it double check that the level is 
DEPTH_CLQ?  Otherwise someone can submit an undefined TL object for 
restore() and it will end up in the queue...

			...jim

On 2/5/16 3:21 PM, Laurent Bourgès wrote:
> 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