<div dir="ltr"><div><div><div><div>Jim & Phil,<br><br></div>Here is an updated webrev according to jim's comments:<br><a href="http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.1/">http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.1/</a><br><br></div>Changes:<br></div>- 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 ?)<br></div>- Improved CrashPaintTest to check few pixel values to ensure rendering is correct<br><div><div><div><br></div><div>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 ?<br></div><div><br></div><div>Regards,<br></div><div>Laurent<br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-02-05 2:20 GMT+01:00 Jim Graham <span dir="ltr"><<a href="mailto:james.graham@oracle.com" target="_blank">james.graham@oracle.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Ah, at least one error spotted here after my initial review...<br>
<br>
The abox values could be changed by the code in lines 157-160 so you can't cache their values until after that.<br>
<br>
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...<span class=""><font color="#888888"><br>
<br>
                ...jim</font></span><div class=""><div class="h5"><br>
<br>
On 2/4/2016 5:10 PM, Jim Graham wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi Laurent,<br>
<br>
In AAShapePipe you load the values from abox[] into variables named<br>
xywh, but these are not xywh values, they are min/max values.  We<br>
typically use any of the following naming conventions for these types of<br>
values:<br>
<br>
- x0, y0, x1, y1<br>
- x1, y1, x2, y2<br>
- minX, minY, maxX, maxY<br>
<br>
Other than that naming inconsistency the changes look great...<br>
<br>
             ...jim<br>
<br>
On 2/4/2016 2:21 PM, Laurent Bourgès wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Please review the webrev fixing SEGFAULT (P2) in the Marlin Renderer<br>
when using thread-local storage with custom Paint (reentrance):<br>
bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8148886" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8148886</a><br>
webrev: <a href="http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.0/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.0/</a><br>
<br>
Changes:<br>
- detect reentrance in MarlinRenderingEngine using flag<br>
RendererContext.usedTL (true/false) and use another context from CLQ<br>
- added few more details in array checks (XXXArrayCache)<br>
- fixed AAShapePipe to support reentrancy: added defensive copy of int[]<br>
abox in local variables + TODO to discuss<br>
- updated Version to 0.7.3.2<br>
- fixed copyright headers<br>
<br>
Best regards,<br>
Laurent<br>
</blockquote></blockquote>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature">-- <br>Laurent Bourgès</div>
</div></div>