Hi David,
I think now that recursion is broken by special-purpose parsing
code-path and, depending on how we solve the other contention point in
j.l.Class.initAnnotationsIfNecessary(), there might be no danger of
dead-lock even if we used double-checked locking idiom in
AnnotationType.getInstance(). We could re-introduce a singleton lock,
which would mean contention when a lot of distinct annotation types need
to be constructed at the same time by multiple threads. Or the
synchronization could be performed on the 'annotationClass' instance,
which is prone to deadlock interaction with user code that synchronizes
on the j.l.Class objects while obtaining annotations.
So you're right: there would either have to be some private lock object
referenced from j.l.Class or an indirection via some kind of Future
which memoizes the result for further invocations and which (the Future)
has to be installed atomically into the j.l.Class field.
But as parsing 2 small annotations and skipping the rest out from a
byte[] of raw annotations is a relatively fast operation (I measured
about 6...8 μs on my i7 PC depending on whether there were lots of other
meta-annotations present on the annotation or not), I think there will
be no problem in wasted CPU cycles even If we keep optimistic
construction. But we'll see.
Regards, Peter
On 07/15/2013 07:45 AM, 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
