bottleneck by java.lang.Class.getAnnotations() - rebased patch

Joe Darcy joe.darcy at
Tue Dec 11 02:14:40 UTC 2012

Hi Peter,

On 12/10/2012 02:23 PM, Peter Levart wrote:
> Hi David, Joe,
> I unpacked the of the first release of JDK 1.5.0. Here's the 
> relevant part:
>     private transient Map<Class, Annotation> annotations;
>     private transient Map<Class, Annotation> declaredAnnotations;
>     private synchronized void initAnnotationsIfNecessary() {
>         if (annotations != null)
>             return;
>         declaredAnnotations = AnnotationParser.parseAnnotations(
>             getRawAnnotations(), getConstantPool(), this);
>         Class<?> superClass = getSuperclass();
>         if (superClass == null) {
>             annotations = declaredAnnotations;
>         } else {
>             annotations = new HashMap<Class, Annotation>();
>             superClass.initAnnotationsIfNecessary();
>             for (Map.Entry<Class, Annotation> e : 
> superClass.annotations.entrySet()) {
>                 Class annotationClass = e.getKey();
>                 if 
> (AnnotationType.getInstance(annotationClass).isInherited())
>                     annotations.put(annotationClass, e.getValue());
>             }
>             annotations.putAll(declaredAnnotations);
>         }
>     }
> ...there's no sign of invalidation logic here yet. Neither for 
> annotations nor for fields/methods/constructors. This appears to have 
> been added later, presumably to accommodate class redefinition 
> changing annotations.
> I would also like to see the source of AnnotationType to see if it 
> used the same logic and synchronization, but that's not part of 
> sources...

In JDK 5 GA, the annotations feature did not attempt to support class 
file definition.  From some quick bug archaeology, that omission was 
addressed circa 5.0u8 (and JDK 6) via bugs including:

     5002251 potential bug with annotations and class file evolution

     6412391 fix for annotation cache and RedefineClasses() conflict 
needs HotSpot changes

     6422541 fix for {Constructor,Field,Method} annotation cache and 
RedefineClasses() conflict needs HS changes


> Regards, Peter
> On 12/10/2012 09:52 PM, Joe Darcy wrote:
>> Yes; the OpenJDK sources only go back to circa 2007, which was 
>> part-way into the JDK 7 release.  The exact changesets of how 
>> annotations were added back in JDK 5 are not available publicly. 
>> However, the final results of that process may be mostly visible in 
>> the from JDK 5.
>> HTH,
>> -Joe
>> On 12/10/2012 8:13 AM, Peter Levart wrote:
>>> Hi David,
>>> It would be informative to look at how java.lang.Class evolved 
>>> during history. The oldest revision I can access is from 1st of Dec. 
>>> 2007, which already contains Java 1.5 code (annotations) and is more 
>>> or less unchanged until now. The most interesting changesets would 
>>> be those that introduced annotations.
>>> Regards, Peter
>>> On 12/10/2012 03:59 PM, Peter Levart wrote:
>>>> On 12/10/2012 07:18 AM, David Holmes wrote:
>>>>> Hi Peter,
>>>>> Sorry for the delay on this.
>>>> Thank you for taking it into consideration. I can only imagine how 
>>>> busy you are with other things.
>>>>> Generally your VolatileData and my ReflectionHelper are doing a 
>>>>> similar job. But I agree with your reasoning that all of the 
>>>>> cached SoftReferences are likely to be cleared at once, and so a 
>>>>> SoftReference to a helper object with direct references, is more 
>>>>> effective than a direct reference to a helper object with 
>>>>> SoftReferences. My initial stance with this was very conservative 
>>>>> as the more change that is introduced the more uncertainty there 
>>>>> is about the impact.
>>>>> I say the above primarily because I think, if I am to proceed with 
>>>>> this, I will need to separate out the general reflection caching 
>>>>> changes from the annotation changes. There are a number of reasons 
>>>>> for this:
>>>>> First, I'm not at all familiar with the implementation of 
>>>>> annotations at the VM or Java level, and the recent changes in 
>>>>> this area just exacerbate my ignorance of the mechanics. So I 
>>>>> don't feel qualified to evaluate that aspect.
>>>>> Second, the bulk of the reflection caching code is simplified by 
>>>>> the fact that due to current constraints on class redefinition the 
>>>>> caching is effectively idempotent for fields/methods/constructors. 
>>>>> But that is not the case for annotations.
>>>> I think that on the Class-level these two aspects can be separated. 
>>>> But not on the Field/Method/Constructor level, because annotations 
>>>> are referenced by Field/Method/Constructor objects and there is no 
>>>> invalidation logic - like on the Class-level - that would just 
>>>> invalidate the sets of annotations on Field/Method/Constructor, 
>>>> leaving Field/Method/Constructor objects still valid. So these two 
>>>> aspects are connected - it may be - I haven't looked at history yet 
>>>> - that invalidation of Field/Method/Constructor was introduced just 
>>>> because of annotations.
>>>> Also, the following bug (currently not accessible): 
>>>> has 
>>>> references to docs that suggest that current class redefinition can 
>>>> introduce new methods, so If this is correct, then redefinition is 
>>>> not idempotent for basic reflection data.
>>>>> Finally, the use of synchronization with the annotations method is 
>>>>> perplexing me. I sent Joe a private email on this but I may as 
>>>>> well raise it here - and I think you have alluded to this in your 
>>>>> earlier emails as well: initAnnotationsIfNecessary() is a 
>>>>> synchronized instance method but I can not find any other code in 
>>>>> the VM that synchronizes on the Class object's monitor. So if this 
>>>>> synchronization is trying to establish consistency in the face of 
>>>>> class redefinition, I do not see where class redefinition is 
>>>>> participating in the synchronization!
>>>> I think that the intent was merely synchronized access to / lazy 
>>>> initialization of cached 'annotations' and 'declaredAnnotations' 
>>>> Maps. Field[], Method[], Constructor[] arrays are published to 
>>>> other threads via volatile fields one field at a time, but 
>>>> initAnnotationsIfNecessary() was designed to publish two references 
>>>> ('annotations' and 'declaredAnnotations') atomically at the same 
>>>> time, so the author of the code choose to use synchronized block. I 
>>>> also have a feeling that this might have simply been the author's 
>>>> preferred style of synchronization, since the same approach is used 
>>>> also in Field/Method/Constructor/Executable although there's just 
>>>> one field of annotations that is published at a time.
>>>> It is indicative that initAnnotationsIfNecessary() synchronized 
>>>> method also contains the call to clearCachesOnClassRedefinition(), 
>>>> so at first it seems that the synchronization is also meant to 
>>>> serialize access to invalidation logic which invalidates both 
>>>> Field/Method/Constructor and annotation fields, but that all 
>>>> falls-apart because clearCachesOnClassRedefinition() is also called 
>>>> from elsewhere, not guarded by the synchronization.
>>>> So all in all the two aspects - annotations and basic reflection 
>>>> stuff - are quite intermingled in current code, unfortunately very 
>>>> inconsistently. I'm afraid that doing one thing and not touching 
>>>> the other is possible, but that would not solve the problems that 
>>>> this thread was starting to address (bottleneck by 
>>>> java.lang.Class.getAnnotations()) and evident dead-lock bugs.
>>>> We can approach the problem so that we separate the aspects of 
>>>> caching Class-level annotations and Field/Method/Constructor arrays 
>>>> but using the same approach for both. And that would only make 
>>>> sense if there was a reason to separately cache Class-level 
>>>> annotations and Field/Method/Constructor arrays. I haven't yet been 
>>>> able to think of one such reason. So anyone with more insight (the 
>>>> author of annotations code?) is invited to participate in 
>>>> investigation.
>>>> My approach of including the Class-level annotations together with 
>>>> Field/Method/Constructor arrays was guided by:
>>>> - consistency - why should Class-level annotations be cached with 
>>>> hard references, while Field/Method/Constructor annotations are 
>>>> indirectly SoftReference(d)? Are they more important?
>>>> - simplicity
>>>> - space efficiency
>>>> - correctness - unsynchronized calls to 
>>>> clearCachesOnClassRedefinition() followed by unsynchronized lazy 
>>>> initialization have - as I have shown - theoretical races that 
>>>> could result in caching the old versions of data instead of new. 
>>>> The approach I have chosen with the logic in getVolatileData() is a 
>>>> kind of MVCC rather than synchronization.
>>>> But as said, the two aspects of caching can be separated.
>>>> We can also leave the Class-level annotation aspect untouched by 
>>>> re-introducing the 'annotations' and 'declaredAnnotations' fields 
>>>> and also the 'lastRedefinedCount' field on the Class-level and 
>>>> re-introducing the synchronized initAnnotationsIfNecessary() method 
>>>> and a clearCachesOnClassRedefinition() which would just invalidate 
>>>> the two annotations fields.
>>>> To recap. How to continue?
>>>> a) as proposed;
>>>> b) separate caching of annotations and Field/Method/Constructor 
>>>> arrays but with the same unblocking MVCC-like approach for both - 
>>>> with possible variation in that annotations be hardly referenced 
>>>> and Field/Method/Constructor arrays be softly;
>>>> c) undo the annotations caching changes and only do MVCC for 
>>>> Field/Method/Constructor arrays.
>>>> I can do b) but prepare two independent patches - one for 
>>>> VolatileData (rename it to MemberData?) and one for AnnotationData. 
>>>> So by applying only the first, we achieve c) and can later apply 
>>>> the second to upgrade to b). But it is unfortunately a) that is 
>>>> most efficient space-saving wise.
>>>> What do you say about the trivial changes in 
>>>> Field/Method/Constructor/Executable to accommodate caching on the 
>>>> 'root' instance instead of on the copies?
>>>> Regards, Peter
>>>>> So what I would like to do is take your basic VolatileData part of 
>>>>> the patch and run with it for JEP-149 purposes, while separating 
>>>>> the annotations issue so they can be dealt with by the experts in 
>>>>> that particular area.
>>>>> I'm sorry it has taken so long to arrive at a fairly negative 
>>>>> position, but I need someone else to take up the annotations 
>>>>> gauntlet and run with it.
>>>>> Thanks,
>>>>> David
>>>>> On 3/12/2012 5:41 PM, Peter Levart wrote:
>>>>>> Hi David, Alan, Alexander and others,
>>>>>> In the meanwhile I have added another annotations space 
>>>>>> optimization to
>>>>>> the patch. If a Class doesn't inherit any annotations from a 
>>>>>> superclass,
>>>>>> which I think is a common case, it assigns the same Map instance to
>>>>>> "annotations" as well as "declaredAnnotations" fields. Previously 
>>>>>> - and
>>>>>> in the original code - this only happened for java.lang.Object and
>>>>>> interfaces.
>>>>>> Here's the updated webrev:
>>>>>> I have also rewritten the performance micro-benchmarks. With the
>>>>>> addition of repeating annotations, one performance aspect 
>>>>>> surfaces: when
>>>>>> asking for a particular annotation type on a Class and that 
>>>>>> annotation
>>>>>> is not present, the new repeating annotations support method
>>>>>> AnnotationSupport.getOneAnnotation asks for @ContainedBy 
>>>>>> meta-annotation
>>>>>> on the annotation type. This can result in an even more apparent
>>>>>> synchronization hot-spot with original code that uses synchronized
>>>>>> initAnnotationsIfNecessary(). This aspect is tested with the 3rd 
>>>>>> test.
>>>>>> Other 2 tests test the same thing as before but are more stable now,
>>>>>> since now they measure retrieval of 5 different annotation types 
>>>>>> from
>>>>>> each AnnotatedElement, previously they only measured retrieval of 1
>>>>>> which was very sensitive to HashMap irregularities (it could 
>>>>>> happen that
>>>>>> a particular key mapped to a bucket that was overloaded in one 
>>>>>> test-run
>>>>>> and not in another)...
>>>>>> Here're the new tests:
>>>>>> And the corresponding results when run on an i7 CPU on Linux:
>>>>>> Regards, Peter
>>>>>> On 12/03/2012 02:15 AM, David Holmes wrote:
>>>>>>> On 1/12/2012 4:54 AM, Alan Bateman wrote:
>>>>>>>> On 30/11/2012 18:36, Peter Levart wrote:
>>>>>>>>> :
>>>>>>>>> So, what do you think? What kind of tests should I prepare in 
>>>>>>>>> addidion
>>>>>>>>> to those 3 so that the patch might get a consideration?
>>>>>>>> I think this is good work and thanks for re-basing your patch. 
>>>>>>>> I know
>>>>>>>> David plans to do a detail review. I think it will require 
>>>>>>>> extensive
>>>>>>>> performance testing too, perhaps with some large applications.
>>>>>>> Indeed I do plan a detailed review and have initiated some initial
>>>>>>> performance tests.
>>>>>>> I am also swamped but will try to get to the review this week - and
>>>>>>> will also need to check the referenced annotations updates.
>>>>>>> David
>>>>>>>> -Alan

More information about the core-libs-dev mailing list