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

Peter Levart peter.levart at
Mon Dec 10 22:23:25 UTC 2012

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)
         declaredAnnotations = AnnotationParser.parseAnnotations(
             getRawAnnotations(), getConstantPool(), this);
         Class<?> superClass = getSuperclass();
         if (superClass == null) {
             annotations = declaredAnnotations;
         } else {
             annotations = new HashMap<Class, Annotation>();
             for (Map.Entry<Class, Annotation> e : 
superClass.annotations.entrySet()) {
                 Class annotationClass = e.getKey();
                     annotations.put(annotationClass, e.getValue());

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

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 

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