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

[Paul Sandoz]

On Aug 12, 2013, at 2:40 PM, Peter Levart <peter.levart at 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.