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
