AnnotationType & AnnotationSupport + repeating annotations (original) (raw)

Peter Levart peter.levart at gmail.com
Mon Dec 3 21:52:36 UTC 2012

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:

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:

private static final ClassValue<ThreadLocal> 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).

More information about the core-libs-dev mailing list