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