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
