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

Mandy Chung mandy.chung at oracle.com
Wed Dec 12 22:59:01 UTC 2012 

Hi Peter,

On 12/12/12 4:31 AM, Peter Levart wrote:
> Hi all,
>
> Ok, no problem. So here's a patch that summarizes what I did in the 
> previous patch, but only for reflective fields (Field[], Method[], 
> Constructor[]) not including annotations:
>
> http://dl.dropbox.com/u/101777488/jdk8-tl/JEP-149.c/webrev/index.html
>

Finally able to make time to review this patch.  It's good work.  While 
it's good to see the synchronization issue with annotations be fixed, 
separating the cache for reflection and annotation helps.  As David 
replied, he will take your patch and run with it for JEP-149.

The change looks good.  Nit: there are several long lines 
L2235,2244,2245,2249,2269 etc that should be broken into separate 
lines.  The remaining open question is the performance difference in the 
reflectionData() method and how well it will be jit'ed.  In the common 
case, there is no class redefinition where 
classCachesOnClassRedefinition() is essentially like an nop.  I believe 
David will look at the footprint and performance numbers as he has 
initiated the performance testing (need to do it with this new patch).

Thanks
Mandy

> The annotations part is unchanged semantically, but I had to:
>
> - modify private method clearCachesOnClassRedefinition to only include 
> invalidation of annotations and declaredAnnotations fields. I also 
> renamed it to clearAnnotationCachesOnClassRedefinition
> - renamed lastRedefinedCount to lastAnnotationsRedefinedCount and, 
> since this field is now only accessed while holding a lock (from 
> synchronized initAnnotationsIfNecessary), I removed the volatile keyword.
>
> That's it.
>
> While looking at this unchanged part of code some more, I see other 
> races as well. For example: two concurrent accesses to annotations 
> combined with redefinition of a class can result in NPE. Here's a 
> serial execution:
>
> Thread 1:
>     getAnnotation(annotationType);
>         initAnnotationsIfNecessary();
>
> VM:
>     classRedefinedCount++;
>
> Thread 2:
>     getAnnotation(annotationType);
>         initAnnotationsIfNecessary();
>             clearAnnotationCachesOnClassRedefinition();
>                 annotations = null;
>
> Thread 1:
>         return AnnotationSupport.getOneAnnotation(annotations, 
> annotationClass);
>         // 'annotations' field can be null
>
>
> So this needs to be fixed sooner or later.
>
> Joel!
>
> Are your JSR 308 canges involving java.lang.Class too?
>
> Regards, Peter
>
>
> On 12/12/2012 11:59 AM, Joel Borggrén-Franck wrote:
>> Hi all,
>>
>> First, thanks all of you that are involved in this!
>>
>> I agree with David here, lets split this up (for now) and do 
>> reflective objects in the context of jep-149 and annotations separately.
>>
>> As you may know there are even more annotation coming in with JSR 308 
>> annotations on type (use), so I want to complete that work first and 
>> then do the effort of reducing contention and overhead for both type 
>> and regular annotations and also fixing up the behaviour for 
>> redefine/retransform class.
>>
>> One other point to consider is that some of the fields in 
>> java/lang/reflect/ classes are known by the VM so not all changes in 
>> Java-land are actually doable. Glancing over your patches very 
>> quickly I don't think you have done anything to upset the VM, but 
>> then I am not an expert in this area.
>>
>> Also, with the VM permgen changes we might have to rethink our 
>> assumptions in order to reduce total overhead. For example as I 
>> understand it previously we could just ship the same pointer into 
>> permgen for the annotations arrays, now that isn't possible so we 
>> create a new copy of the array for every Field/Method/Constructor 
>> instance. Perhaps there is some clever way of eliminating those copies.
>>
>> So while I think your patches generally makes sense, I think it is 
>> prudent to delay this for annotations until all our new annotation 
>> features are in.
>>
>> cheers
>> /Joel
>>
>> On Dec 10, 2012, at 7:18 AM, David Holmes <david.holmes at oracle.com> 
>> wrote:
>>
>>> Hi Peter,
>>>
>>> Sorry for the delay on this.
>>>
>>> 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.
>>>
>>> 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!
>>>
>>> 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