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.gra...@oracle.com
<mailto:james.gra...@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
