On 07/04/2013 07:34 PM, Joel Borggrén-Franck wrote:
Also, can you please explain to me why the update race is safe. I have done 
the/some research myself but I would like to know which angles you have covered.
Well, one thing is that AnnotationType class is now effectively final, meaning that all 
it's state is referenced using final fields. If a reference to an instance of such class 
is obtained via data-race, all it's state is observed consistent by the thread that 
obtained the reference. The other thing is racy caching. If two or more threads are 
concurrently requesting AnnotationType for the same Class, many of them might construct 
and use it's own instance and the one published "latest" will finally be the 
one being cached. Since all such AnnotationType instances are constructed from the same 
input data, they are equivalent and it doesn't matter which one is used by which thread.

Actually they aren't, something have been bugging me and I finally figured it 
out today. The problem is with default valued elements. Previously all threads 
have been seeing the same instance of default values but now that will only be 
guaranteed for Classes, Strings and Integers inside the value cache. While I 
don't think it is in the spec that annotation default value instances should be 
== for all threads I don't think it is a race we should introduce. Given this I 
think you should use some approach to produce a winner that every thread will 
use (like in the other annotations patch). My gut feeling is that CASing in a 
result will be better under contention that the current lock solution (while 
also correct and deadlock free) for a net win.

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

Reply via email to