cc'ing Joel Borggrén-Franck as it he added the support for repeating
annotations very recently.
On 04/12/2012 09:23, Peter Levart wrote:
Hi again,
Zhong Yu suggested to use plain TheadLocal<Map<Class, ...>> instead of
ClassValue<ThreadLocal> in the proposed patch. Here's the updated
webrev that contains this change:
http://dl.dropbox.com/u/101777488/jdk8-tl/AnnotationTypeSupport/webrev.02/index.html
The improvement is twofold:
- There is no ClassValue.ClassValueMap installed on the annotation
Class instances
- The mechanism for exposing an instance of AnnotationType for
recursive calls in the same thread only uses the ThreadLocal entry for
the in-flight recursive call and cleans-up and removes the entry when
the call stack unwinds, so no unused retained objects are left behind.
The results of the benchmarks are not affected.
Regards, Peter
On 12/03/2012 10:52 PM, Peter Levart wrote:
Hi David and Alan,
The following is in part connected with the new code for repeating
annotations, and in part with the proposed patch for resolving
synchronization issues with annotations as well as improvements
regarding space and multi-threaded contention.
Although connected, the patch proposed here can be taken entirely by
itself. Let me try to explain what this patch does. This patch
removes the "synchronized" keyword from the static method
AnnotationType.getInstance(Class). By itself this synchronization
does not present any big performance issue since the method is always
called on slow-path (when parsing the annotations, when lazily
constructing a Map of inherited annotations, when de-serializing
annotations). The reason this method is synchronized on a single
monitor is because it:
- lazily constructs an instance of AnnotationType and exposes it to
other threads via a Class.annotationType field
- in the middle of the AnnotationType constructor it exposes a
half-constructed instance via a Class.annotationType field to current
thread so that recursive calls don't result in infinite recursion.
As current parsing/resolving logic is coded, other threads must not
see the half-constructed AnnotationType instance and current thread
must see it. This is achieved by single re-entrant lock because only
single lock guarantees the absence of dead-locks (as can be seen from
bugs this lock in combination with initAnnotationsIfNecessary() is a
cause for dead-locks, but that's another story).
Now because there is a single lock, the method must not be called on
heavily executed code paths or this will present a synchronization
bottleneck. One such heavily executed code path is in the new
sun.reflect.annotation.AnnotationSupport class that contains the
logic for repeating annotations. In this class the AnnotationType for
a particular annotation class is not obtained via this synchronized
method, but incorrectly via direct unsynchronized read of
Class.annotationType field. The code in AnnotationSupport can
therefore dereference a half-constructed AnnotationType instance
before it's constructor, executing in another thread, is finished and
before final fields in object are frozen.
Class.[get,set]AnnotationType should only be called from within the
synchronized AnnotationType.getInstance method, but that apparently
is to contended to be practical.
I solved the problem by:
- making AnnotationType an immutable object (all fields final)
- exposing the instance to other threads via an unsynchronized write
to Class.annotationType field only after constructor is finished and
final fields are frozen
- exposing the instance to current thread for recursive calls in the
middle of the constructor via a special:
private static final ClassValue<ThreadLocal<AnnotationType>>
IN_CONSTRUCTION = ...
field.
It's true, this does present an overhead in storage, since every
Class instance for annotation type will have a
ClassValue.ClassValueMap installed, but it is hoped that the number
of different annotation classes is not big compared to the number of
all classes. A ThreadLocal referenced by ClassValue is only set for
the in-flight recursive call and cleared afterwards.
Because with such non-blocking access to AnnotationType,
AnnotationType.getInstance() can be used in AnnotationSupport
properly to quickly access the AnnotationType instances. The access
to AnnotationType with this patch is so quick that I added 2 fields
to it (container, containee) where the container and/or containee for
a particular annotation type are cached and used in AnnotationSupport
to resolve repeating annotations. This is much faster than invoking
Class.getDirectDeclaredAnnotation() which is a HashMap.get, followed
by reflective invocation of the "value" method on that annotation.
The patch is here:
http://dl.dropbox.com/u/101777488/jdk8-tl/AnnotationTypeSupport/webrev.01/index.html
The benchmarks that show improvement are the same benchmarks used in
my related proposed patch (Class.getAnnotations() bottleneck):
https://raw.github.com/plevart/jdk8-tl/JEP-149/test/src/test/ReflectionTest.java
The results are combined results using plain JDK8 code with repeating
annotation and then one patch applied at a time and finally both
patches combined:
https://raw.github.com/plevart/jdk8-tl/JEP-149/test/benchmark_results_i7-2600K.txt
Regards, Peter
P.S.
Maybe this is not the best approach. Another approach would be to
construct a special non-recursive variant of annotation parsing logic
that would be used only for select meta-annotations - just enough to
construct an AnnotationType with all it's data.
The proposed patch is trying to keep the semantics of original logic,
which is not entirely correct. The patch is not trying to solve the
logic in-correctness. Here's a contrived example that exposes the
in-correctness:
Let's have the following two annotations:
@Retention(RetentionPolicy.RUNTIME)
@RuntimeAnnotationB
public @interface RuntimeAnnotationA {}
@Retention(RetentionPolicy.RUNTIME)
@RuntimeAnnotationA
public @interface RuntimeAnnotationB {}
and the following test:
@RuntimeAnnotationA
public class AnnotationRetentionTest {
public static void main(String[] args) {
RuntimeAnnotationA ann1 =
AnnotationRetentionTest.class.getDeclaredAnnotation(RuntimeAnnotationA.class);
System.out.println(ann1 != null);
RuntimeAnnotationA ann2 =
RuntimeAnnotationB.class.getDeclaredAnnotation(RuntimeAnnotationA.class);
System.out.println(ann2 != null);
}
}
The test, when run, prints:
true
true
Now change the RuntimeAnnotationA's retention policy to:
@Retention(RetentionPolicy.CLASS)
@RuntimeAnnotationB
public @interface RuntimeAnnotationA {}
...and only recompile the RuntimeAnnotationA.
When the test is run again with the recompiled RuntimeAnnotationA it
prints:
false
true
Although the annotation data for RuntimeAnnotationA is present in
both AnnotationRetentionTest and RuntimeAnnotationB class files, the
order of initialization and recursion causes the RuntimeAnnotationA
to be loaded as RUNTIME annotation into the RuntimeAnnotationB.class
(WRONG) but not into the AnnotationRetentionTest.class (CORRECT).
