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

[Jim Graham]

Hi Laurent,

This looks great!

The classes you call *Wrapper<K> are really more like factories or 
accessors.  A wrapper would imply that it houses the object in question, 
but these are more concerned with providing an access pattern and/or a 
factory for Reference types to wrap the objects in question.  I would 
probably have named them *Factory instead.

About the only simplifying suggestion I might have is that if you can 
subclass Reference and create HardReference<T> extends Reference<T> with 
an obvious implementation, then you wouldn't have to special case the 
HARD type and you could get rid of a lot of your use of raw "Object" 
references and replace them with generified code.  Unfortunately 
Reference is not subclassable outside the java.lang.ref package.  I 
looked online and found a number of people considering subclassing a 
HardReference variant off of WeakReference - as long as the subclass 
keeps a hard reference the referent in the super class is not relevant. 
  That mechanism may be a little clunky, but it would mean that you can 
always assume that the TL.get() and CLQ.poll() are returning a 
Reference<K> and just call get() on it without having to use a 
factory/accessor.

public class HardReference<T> extends WeakReference<T> {
     private final T strongRef;
     public HardReference(T obj) {
         super(null);
         this.strongRef = obj;
     }
     public T get() { return strongRef; }
}

Another way to crack that egg would be to simply embed the following 
code where you call the resolveReference methods:
     ctx = (obj instanceof Reference<K>) ? ((Reference<K>) obj).get() : 
((K) obj);
(No need for a null test there because instanceof ensures non-nullness 
and you don't care about which form of reference it is since they all 
share the same get() method.)

If you made either of those changes then the Wrapper classes would only 
exist to create the appropriate Reference object and could be left as 
factories or simply be replaced with a wrap(ctx) method in the parent 
with a switch statement in it.  Since it is only called on construction, 
the switch statement should have negligible overhead and eliminate 4 
inner classes (parent Wrapper and 3 flavors of Wrapper).

			...jim

On 2/9/16 2:10 PM, Laurent Bourgès wrote:
> 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
> <mailto: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