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

Wed Dec 12 15:49:57 UTC 2012

On 12/12/2012 04:34 PM, Peter Levart wrote:
> 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.
> You mean the "byte[] annotations" arrays in the 
> Field/Method/Constructor instances returned by the following 
> java.lang.Class native methods:
>
>     private native Field[]       getDeclaredFields0(boolean publicOnly);
>     private native Method[]      getDeclaredMethods0(boolean publicOnly);
>     private native Constructor<T>[] getDeclaredConstructors0(boolean 
> publicOnly);
>
> If caching is effective (useCaches == true) then, in the absence of 
> class redefinition, each of these methods is called at most two times 
> (once for each value of publicOnly flag). If I understand the 
> semantics of this flag correctly, it is used solely for the purpose of 
> filtering the returned instances to include all or only public members.
>
> The java part of caching could be modified to only call each of these 
> methods once (with publicOnly==false) and derive the "publicOnly" 
> variant of the array by doing the filtering in java. This way only a 
> single instance of a particular Field/Method/Constructor will be 
> retained by caches and there will be no duplication of annotation arrays.

Even more, when raw annotations are parsed in the 'root' instance and 
turned into a Map<Class, Annotation>, the raw annotations array can be 
released since it's not needed any more. And on the copies (those that 
are passed to the outside world), there's no need to hold a reference to 
it in the first place (if delegating requests to root), so it can be 
left as null from the beginning.

Regards, Peter

>
> Regards, Peter
>>
>> 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
>>>>>>
>