AnnotationType & AnnotationSupport + repeating annotations

[Joel Borggrén-Franck]

Hi,

I've filed a bug for this,

http://bugs.sun.com/view_bug.do?bug_id=8004919

should hopefully be visible shortly.

I won't get around to fixing this just yet, but thanks for both the investigation and the suggested fix!

cheers
/Joel

On Dec 4, 2012, at 10:23 AM, Peter Levart <peter.levart at gmail.com> 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).
>> 
>