Peter,
Many thanks for preparing that patch. As we're leaving annotations
behind lets continue any further discussion of this patch in a new email
thread specific to JEP-149.
I'll run this patch through some basic testing and performance benchmarks.
The JEP will also need updating so I'll be in touch to confirm details
about size savings etc for the write up.
Thanks again,
David
On 12/12/2012 10:31 PM, Peter Levart wrote:
Hi all,
Ok, no problem. So here's a patch that summarizes what I did in the
previous patch, but only for reflective fields (Field[], Method[],
Constructor[]) not including annotations:
http://dl.dropbox.com/u/101777488/jdk8-tl/JEP-149.c/webrev/index.html
The annotations part is unchanged semantically, but I had to:
- modify private method clearCachesOnClassRedefinition to only include
invalidation of annotations and declaredAnnotations fields. I also
renamed it to clearAnnotationCachesOnClassRedefinition
- renamed lastRedefinedCountto lastAnnotationsRedefinedCountand, since
this field is now only accessed while holding a lock (from synchronized
initAnnotationsIfNecessary), I removed the volatile keyword.
That's it.
While looking at this unchanged part of code some more, I see other
races as well. For example: two concurrent accesses to annotations
combined with redefinition of a class can result in NPE. Here's a serial
execution:
Thread 1:
getAnnotation(annotationType);
initAnnotationsIfNecessary();
VM:
classRedefinedCount++;
Thread 2:
getAnnotation(annotationType);
initAnnotationsIfNecessary();
clearAnnotationCachesOnClassRedefinition();
annotations = null;
Thread 1:
return AnnotationSupport.getOneAnnotation(annotations, annotationClass);
// 'annotations' field can be null
So this needs to be fixed sooner or later.
Joel!
Are your JSR 308 canges involving java.lang.Class too?
Regards, Peter
On 12/12/2012 11:59 AM, Joel Borggrén-Franck wrote:
Hi all,
First, thanks all of you that are involved in this!
I agree with David here, lets split this up (for now) and do
reflective objects in the context of jep-149 and annotations separately.
As you may know there are even more annotation coming in with JSR 308
annotations on type (use), so I want to complete that work first and
then do the effort of reducing contention and overhead for both type
and regular annotations and also fixing up the behaviour for
redefine/retransform class.
One other point to consider is that some of the fields in
java/lang/reflect/ classes are known by the VM so not all changes in
Java-land are actually doable. Glancing over your patches very quickly
I don't think you have done anything to upset the VM, but then I am
not an expert in this area.
Also, with the VM permgen changes we might have to rethink our
assumptions in order to reduce total overhead. For example as I
understand it previously we could just ship the same pointer into
permgen for the annotations arrays, now that isn't possible so we
create a new copy of the array for every Field/Method/Constructor
instance. Perhaps there is some clever way of eliminating those copies.
So while I think your patches generally makes sense, I think it is
prudent to delay this for annotations until all our new annotation
features are in.
cheers
/Joel
On Dec 10, 2012, at 7:18 AM, David Holmes <david.hol...@oracle.com>
wrote:
Hi Peter,
Sorry for the delay on this.
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.
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!
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
