On Aug 12, 2013, at 2:40 PM, Peter Levart <peter.lev...@gmail.com> wrote:
> Hi Joel, > > Thanks for review. Comments inline... > > On 08/12/2013 12:54 PM, Joel Borggren-Franck wrote: >> 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. > > The AnnotationData created and returned concurrently with class redefinition > can contain old or new version, yes, but that's not a problem, since the > AnnotationData also contains a "redefinedCount" field, so any further > requests for annotations will trigger re-computation. This assumes that VM > either changes the state returned by getRawAnnotations() and > "classRedefinedCount" field atomically (during a safepoint?) or at least in > order: 1st getRawAnnotations(), 2nd "classRedefinedCount". We 1st read > "classRedefinedCount" and then > getRawAnnotations(), so there's no danger of keeping and returning stale data > after redefinition. > > This is more or less the same as with ReflectionData caching. > I suppose one could do the cas without checking it's result: 3296 private AnnotationData annotationData() { 3297 while (true) { // retry loop 3298 AnnotationData annotationData = this.annotationData; 3299 int classRedefinedCount = this.classRedefinedCount; 3300 if (annotationData != null && 3301 annotationData.redefinedCount == classRedefinedCount) { 3302 return annotationData; 3303 } 3304 // null or stale annotationData -> optimistically create new instance 3306 // try to install it 3307 Atomic.casAnnotationData(this, annotationData, createAnnotationData(classRedefinedCount)); // re-check conditions in case a re-defintion occurred while creating 3311 } 3312 } Don't really know if it is worth it though, plus it might be better to break out the loop once set. Note frameworks, such as JAXB and JAX-RS will cache results of processing reflection and annotation information and need to be given a kick to re-process (e.g. like from JRebel). >> >> 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? > > Inconsistencies remain that are hard to solve: > > - inherited annotations. They are combined with declared annotations in a Map > that is cached in the AnnotationData. If superclass is redefined, the > inherited annotations are not invalidated. Unless the VM increments > classRedefinedCount for the redefined class and all currently loaded > subclasses. I don't know if this is the case. Same here. There is a comment which implies it does get updated: 2389 // Incremented by the VM on each call to JVM TI RedefineClasses() 2390 // that redefines this class or a superclass. 2391 private volatile transient int classRedefinedCount = 0; Paul.
