bottleneck by java.lang.Class.getAnnotations() - rebased patch
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.
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.
>>> 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
>>>> which I think is a common case, it assigns the same Map instance to
>>>> "annotations" as well as "declaredAnnotations" fields. Previously -
>>>> in the original code - this only happened for java.lang.Object and
>>>> Here's the updated webrev:
>>>> I have also rewritten the performance micro-benchmarks. With the
>>>> addition of repeating annotations, one performance aspect surfaces:
>>>> 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
>>>> 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
>>>> a particular key mapped to a bucket that was overloaded in one
>>>> 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
>>>>>>> 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
>>>>>> 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.
More information about the core-libs-dev