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.lev...@gmail.com > >><mailto:peter.lev...@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.fra...@oracle.com > >><mailto:joel.fra...@oracle.com>> > >>*Cc: *Alan Bateman <alan.bate...@oracle.com > >><mailto:alan.bate...@oracle.com>>, "core-libs-dev@openjdk.java.net > >><mailto:core-libs-dev@openjdk.java.net> core-libs-dev" > >><core-libs-dev@openjdk.java.net <mailto:core-libs-dev@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.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 > >> > >
