RFR 7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Joel Borggrén-Franck]

Thanks for the review David,

I have pushed version 5 of the patch.

cheers
/Joel

On 2013-07-15, David Holmes wrote:
> Joel, Peter,
> 
> I've looked at the synchronization changes and the use of CAS and it
> seems sound.
> 
> The only performance-related concern is the wasted effort when
> multiple threads each call "new AnnotationType(annotationClass)".
> But if that turns out to be an issue we can address it (using
> memoization pattern that installs a Future for the AnnotationType).
> 
> Thanks,
> David
> 
> >>*From: *Peter Levart <peter.levart at gmail.com
> >><mailto:peter.levart at gmail.com>>
> >>*Subject: **RFR 7122142 : (ann) Race condition between
> >>isAnnotationPresent and getAnnotations*
> >>*Date: *8 juli 2013 22:54:12 CEST
> >>*To: *Joel Borggrén-Franck <joel.franck at oracle.com
> >><mailto:joel.franck at oracle.com>>
> >>*Cc: *Alan Bateman <Alan.Bateman at oracle.com
> >><mailto:Alan.Bateman at oracle.com>>, "core-libs-dev at openjdk.java.net
> >><mailto:core-libs-dev at openjdk.java.net> core-libs-dev"
> >><core-libs-dev at openjdk.java.net <mailto:core-libs-dev at openjdk.java.net>>
> >>
> >>Helo,
> >>
> >>I need a Reviewer for the following patch:
> >>
> >>http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/
> >>
> >>This fixes deadlock situation described in bug:
> >>
> >>http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7122142
> >>
> >>The in-depth evaluation of the patch is described in the introductory
> >>message of this thread:
> >>
> >>http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/018226.html
> >>
> >>The above is the 5th revision of the patch that also makes sure a
> >>single AnnotationType instance is ever returned for the same Class
> >>object even when multiple threads concurrently request one.
> >>
> >>
> >>Regards, Peter
> >>
> >>On 07/05/2013 04:04 PM, Joel Borggrén-Franck wrote:
> >>>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.levart at 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
> >>
> >