[OpenJDK 2D-Dev] RFR 8148886: SEGV in sun.java2d.marlin.Renderer._endRendering
james.graham at oracle.com
Mon Feb 8 22:33:48 UTC 2016
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...
On 2/5/16 3:21 PM, Laurent Bourgès wrote:
> Jim & Phil,
> Here is an updated webrev according to jim's comments:
> - 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 ?
> 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...
> 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
> - x0, y0, x1, y1
> - x1, y1, x2, y2
> - minX, minY, maxX, maxY
> Other than that naming inconsistency the changes look great...
> On 2/4/2016 2:21 PM, Laurent Bourgès wrote:
> Please review the webrev fixing SEGFAULT (P2) in the Marlin
> when using thread-local storage with custom Paint (reentrance):
> bug: https://bugs.openjdk.java.net/browse/JDK-8148886
> - 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 Bourgès
More information about the 2d-dev