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
