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
