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

[David Holmes]

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
>>
>