AnnotationType & AnnotationSupport + repeating annotations

[Peter Levart]

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).