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
