AnnotationType & AnnotationSupport + repeating annotations

[Alan Bateman]

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