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

[Jim Graham]

It all looks great.  The comments are fine for internal documentation, 
but here is a suggestion for the initial comment on a couple of the 
Provider classes.  I don't need to review the changes to comments.

(NOTE: Watch out for trailing white space on these if you cut and past 
them back as the process of cutting and pasting the original comments 
from your webrev into my email buffer may have added trailing spaces...)

/**
  * This abstract ReentrantContextProvider helper class manages the 
creation,
  * storage, and retrieval of concrete ReentrantContext instances which 
can be
  * subclassed to hold cached contextual data.
  *
  * It supports reentrancy as every call to acquire() provides a new 
unique context
  * instance that must later be returned for reuse by a call to release(ctx)
  * (typically in a try/finally block).
  *
  * It has a couple of abstract implementations which store references 
in a queue
  * and/or thread-local storage.
  * The Providers can be configured to hold ReentrantContext instances 
in memory
  * using hard, soft or weak references.
  *
  * The acquire() and release() methods are used to retrieve and return 
the contexts.
  *
  * The {@code newContext()} method remains abstract in all 
implementations and
  * must be provided by the module to create a new subclass of 
ReentrantContext
  * with the appropriate contextual data in it.
  *
  * Sample Usage:
    ...

(And probably add a back pointer to the context class at the end of that 
comment:)

  * @see ReentrantContext
  */

Some suggestions on RCPTL:

/**
  * This ReentrantContextProvider implementation uses a ThreadLocal to hold
  * the first ReentrantContext per thread and a 
ReentrantContextProviderCLQ to
  * store child ReentrantContext instances needed during recursion.
  *
  * Note: this implementation may keep up to one context in memory per 
thread.
  * Child contexts for recursive uses are stored in the queue using a WEAK
  * reference by default unless specified in the 2 argument constructor.
  *
  * @param <K> ReentrantContext subclass
  */

				...jim

On 2/10/16 2:25 PM, Laurent Bourgès wrote:
> Jim,
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~lbourges/marlin/marlin-8148886.4/
>
> Changes:
> - HardReference adopted: it simplified the code as I removed the wrapper
> / factory classes but also allowed me to apply generic types:
> Reference<? extends ReentrantContext> instead of Object
> - ReentrantContextProviderTL: added a constructor with 2 reference types
> (TL / CLQ), the default one uses weak reference for CLQ
> - added javadoc in ReentrantContext* classes: please correct my english
> sentences !
> I am not fluent enough to write clearly such documentation and I do not
> know how much details are needed or not as it is a internal API.
>
>     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.
>
>
> That's absolutely what I always needed !
> Several times I wondered why no such HardReference exists in the
> java.lang.reference package:
> should we tell core-libs that it is really needed to improve homogeneity
> / consistency ... and simplify such reference-based code ?
>
>     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; }
>     }
>
>
> I added such class in ReentrantContextProvider but I did not use
> 'super(null)'. What is the aim ?
> Does it simplify the reference processing ? Of course such HardReference
> does not need any reference queue...
>
>     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.)
>
>
> It was the case before in MarlinRenderingEngine but I wanted to avoid
> most conditional statements ... .
>
>     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).
>
>
> Wrapped in ReentrantContextProvider.getOrCreateReference(ctx).
>
> Thanks for your suggestions,
> Laurent
>