AnnotationType & AnnotationSupport + repeating annotations

[Peter Levart]

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