bottleneck by java.lang.Class.getAnnotations() - rebased patch
Joe Darcy
joe.darcy at oracle.com
Tue Dec 11 02:14:40 UTC 2012
Hi Peter,
On 12/10/2012 02:23 PM, Peter Levart wrote:
> Hi David, Joe,
>
> I unpacked the src.zip 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)
> return;
> declaredAnnotations = AnnotationParser.parseAnnotations(
> getRawAnnotations(), getConstantPool(), this);
> Class<?> superClass = getSuperclass();
> if (superClass == null) {
> annotations = declaredAnnotations;
> } else {
> annotations = new HashMap<Class, Annotation>();
> superClass.initAnnotationsIfNecessary();
> for (Map.Entry<Class, Annotation> e :
> superClass.annotations.entrySet()) {
> Class annotationClass = e.getKey();
> if
> (AnnotationType.getInstance(annotationClass).isInherited())
> annotations.put(annotationClass, e.getValue());
> }
> annotations.putAll(declaredAnnotations);
> }
> }
>
>
> ...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 annotations.
>
> 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
> src.zip sources...
In JDK 5 GA, the annotations feature did not attempt to support class
file definition. From some quick bug archaeology, that omission was
addressed circa 5.0u8 (and JDK 6) via bugs including:
5002251 potential bug with annotations and class file evolution
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5002251
6412391 fix for annotation cache and RedefineClasses() conflict
needs HotSpot changes
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6412391
6422541 fix for {Constructor,Field,Method} annotation cache and
RedefineClasses() conflict needs HS changes
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6422541
-Joe
>
> 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 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