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