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

[Peter Levart]

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 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. If this is not the case, 
we could keep both classRedefinedCount and superclassRedefinedCount in 
the AnnotationData and invalidate it if any of them changes. This would 
slow-down any access to annotations though.

- annotation (@interface)  declarations can themselves be redefined (for 
example, defaults changed). Such redefinitions don't affect already 
initialized annotations. Default values are cached in AnnotationType 
which is not invalidated as a result of class redefinition. Maybe it 
should be. And even if AnnotationType was invalidated as a result of 
class redefinition, the defaults are looked-up when particular 
annotations are initialized and then combined with parsed values in a 
single values map inside each annotation instance (proxy), so 
invalidating AnnotationType would have no effect on those annotations.

>
> 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.

Right, AnnotationParser does return LinkedHashMap, so at least 
decalredAnnotations are in parse-order. I will change the code to use 
LinkedHashMap for inherited/combined case too. The number of annotations 
that end-up in inherited/combined Map is not known in advance. I could 
make a separate pre-scan for superclass annotations that are 
@Inheritable and don't have the same key as any of declared annotations 
and then sum this count with declared annotations count, but I don't 
think this will be very effective. I could allocate a large-enough Map 
to always fit (the count of superclass annotations + the count of 
declared annotations), but that could sometimes lead to much 
over-allocated Maps. I could take the min(DEFAULT_CAPACITY, superclass 
annotations count + declared annotations count) as the initial capacity 
for the Map. What do you think which of those 3 alternatives is the best?

>
> 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?

I successfully ran the jdk_lang jtreg tests which contain several tests 
for annotations.

>
> Otherwise this looks good (not an R kind of reviewer).
>
> cheers
> /Joel

Regards, Peter