[OpenJDK 2D-Dev] RFR 8148886: SEGV in sun.java2d.marlin.Renderer._endRendering

[Laurent Bourgès]

Jim,

Thanks to your comments, I generalized the TL / CLQ approach in sun.java2d
package with new classes:
- ReentrantContext: base class for thread contexts with few package-visible
fields: usage and reference
- ReentrantContextProvider: base provider class that define the abstract
methods to be implemented + several reference wrappers (hard, soft, weak)
- ReentrantContextProviderCLQ: very simple provider that only use a CLQ for
storage
- ReentrantContextProviderTL: enhanced Thread Local provider that uses
another ReentrantContextProviderCLQ if reentrance detected

These 2 last implementations supports any reference wrapper as given in
their constructor.

Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.3/

Please give me your point of view; is it the good direction ?
(javadoc / comments remain to be fixed)

Of course, the new classes are used by both AAShapePipe and
MarlinRenderingEngine and it works well:
performance is still the same but all tests are passing.


Few anwsers below:

2016-02-08 23:33 GMT+01:00 Jim Graham <james.graham at oracle.com>:

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

Done as proposed; the 'hurdles' were that I want to keep high performance
and may prefer sometimes code duplication ... but my first tests seem
indicating that the new approach is equivalent as before.


> MarlinRenderingEngine line 182 - testing useThreadLocal is
> redundant/irrelevant here, checking the queue variable should be enough.
>

(it was a trick as hotspot optimizes dead code: the condition
!useThreadLocal may let hotspot remove a useless block)


> ReentrantThreadLocal - depth is a poor name for that field.  Perhaps
> usage?  With "*_TL_INACTIVE", "*_TL_IN_USE", and "*_CLQ" being the variants?
>

Agreed: I adopted your proposal.


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

I added the double-check as recommended.

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20160209/09d742d1/attachment.html>