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
