Hi Peter, Thanks for the quick update!
While I have looked over the changes to j.l.Class and the cas in AnnotationType I don't think I'm qualified to review that. (FWIW it looked fine to me but my jmm isn't swapped in at the moment so I won't pretend to know the interactions between volatile and Unsafe cas). Thinking out loud I suppose we can assume a stable offset of fields in Class, and I realize that part has been under review before. The rest of AnnotationType, AnnotationParser and the tests look fine though. I also ran the tests before and after the change and results were as expected. Since I'm not a Reviewer kind of reviewer you need to get one those to sign off but after that I think you are good to go. I can sponsor this as well if Alan is busy. cheers /Joel On 5 jul 2013, at 11:12, Peter Levart <peter.lev...@gmail.com> wrote: > Hi Again, > > Sorry, the 4th revision I posted is not rebased to the current tip of jdk8-tl > so it contained some diffs that reverted some things. > > Here's the correct patch: > > http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/ > > > Regards, Peter > > > On 07/05/2013 10:32 AM, Peter Levart wrote: >> Hi Joel, >> >> Here's the 4th revision of the patch: >> >> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.04/ >> >> This one introduces CAS-ing of the AnnotationType instance into the >> Class.annotationType field so that there's only a single instance ever >> returned for a single Class. I also introduced new private static >> Class.Atomic nested class that is used to lazily initialize Unsafe machinery >> and to provide a safe wrapper for internal j.l.Class use. Previously this >> was part of ReflectionData nested class because it was only used for it's >> purpose. In anticipation to have a need for more atomic operations in the >> future, I moved this code to a separate class. ReflectionData is now just a >> record. >> >> Regards, Peter >
