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