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
