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 >>>> >>
