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

Joe Darcy joe.darcy at oracle.com
Mon Dec 10 20:52:57 UTC 2012 

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 src.zip 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): 
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5002251 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:
>>>>
>>>> http://dl.dropbox.com/u/101777488/jdk8-tl/JEP-149/webrev.02/index.html
>>>>
>>>> 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:
>>>>
>>>> https://raw.github.com/plevart/jdk8-tl/JEP-149/test/src/test/ReflectionTest.java 
>>>>
>>>>
>>>> And the corresponding results when run on an i7 CPU on Linux:
>>>>
>>>> https://raw.github.com/plevart/jdk8-tl/JEP-149/test/benchmark_results_i7-2600K.txt 
>>>>
>>>>
>>>>
>>>> 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