RFR JDK-8011940 : java.lang.Class.getAnnotations() always enters synchronized method

[Joel Borggren-Franck]

Hi Peter,

Thank you for looking in to this!

On 2013-08-11, Peter Levart wrote:
> 
> On 08/07/2013 06:42 PM, Aleksey Shipilev wrote:
> >Hi Peter,
> >
> >On 08/07/2013 08:18 PM, Peter Levart wrote:
> >>http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.01/
> >Yeah, looks familiar. The install loop is very complicated though, can
> >we simplify it? It seems profitable to move the retry loop up into
> >annotationData(): you then don't have to pass the $annotationData, you
> >can merge the exit paths for $classRedefinedCount, etc.
> 
> Hi Aleksey,
> 
> Right, the retry-loop can be simplified. There are still two
> methods, but the 2nd is only a factory method now:
> 
> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.02/
> 

I realize the interaction between probably any reflective operation and
a redefine is blurry, but if a redefine occurs between line 3308 and 3309
annotationData() will return a possibly outdated AnnotationData. 

3307             if (Atomic.casAnnotationData(this, annotationData, newAnnotationData)) {
3308                 // successfully installed new AnnotationData

redefine here

3309                 return newAnnotationData;
3310             }

I suppose there is a sequencing of events where this will be inevitable,
but this got me thinking about the state model of annotations (and
reflective objects) through an redefine.

I think we need to be a bit more explicit about documenting that state
model. If you have a good mental model of this perhaps now is a good
time to write it down?

3326                 if (annotations == null) { // lazy construction
3327                     annotations = new HashMap<>();
3328                 }

I think this should be a LinkedHashMap, so that iteration is predictable
(I know it isn't in the current code). Also the size of the map is known
so you can use a constructor with an explicit initial capacity.

Since this is a fairly significant rewrite I think it might be good to
make sure our tests exercise the new code. Can you run some kind of
coverage report on this?

Otherwise this looks good (not an R kind of reviewer).

cheers
/Joel