bottleneck by java.lang.Class.getAnnotations() - rebased patch
Peter Levart
peter.levart at gmail.com
Mon Dec 10 16:41:07 UTC 2012
Ok, I've got it. Downloaded j2sdk1.4_19 and unpacked src.zip ...
There are SoftReferences for individual Field/Method/Constructor arrays:
// Caches for certain reflective results
private static boolean useCaches = true;
private volatile transient SoftReference declaredFields;
private volatile transient SoftReference publicFields;
private volatile transient SoftReference declaredMethods;
private volatile transient SoftReference publicMethods;
private volatile transient SoftReference declaredConstructors;
private volatile transient SoftReference publicConstructors;
// Intermediate results for getFields and getMethods
private volatile transient SoftReference declaredPublicFields;
private volatile transient SoftReference declaredPublicMethods;
...but there's no mechanism yet for invalidation. So my assumption that
invalidation was introduced just because of annotations might be
correct. Annotations were just glued on top of the existing unchanged
structures.
Regards, Peter
P.S. I've got an Idea how to approach another variant where Class-level
annotations would still be directly referenced from the Class instance
and Field/Method/Constructor arrays would hang off a single
SoftReference in a way that would have same space efficiency that my
current integrated approach. The idea is to subclass the SoftReference
and have the additional fields on it:
static class VolatileData extends SoftReference<MemberData> {
volatile Map annotations, declaredAnnotations;
final int redefinedCount;
}
...and MemberData only containing volatile Fileld[], Method[],
Constructor[] fields...
I also know how to correctly synchronize access to this structure so
that annotations are not invalidated when SoftReference is cleared. Let
this be variant a2).
I rest now and wait for your pointers.
Regards, Peter
On 12/10/2012 05:13 PM, 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