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 <[email protected]>: > 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 <[email protected]>: > >> 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
