bottleneck by java.lang.Class.getAnnotations() - rebased patch
Peter Levart
peter.levart at gmail.com
Mon Dec 10 22:23:25 UTC 2012
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...
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