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 <[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
