Hi Peter, Thanks for this, please add a "@bug 8011940" tag to your test. IMO you don't need a re-review for that. Otherwise looks good.
We still need a Reviewer, Chris, you reviewed a previous version, can you look at this one too? cheers /Joel On 27 aug 2013, at 15:00, Peter Levart <peter.lev...@gmail.com> wrote: > Hi Joel and others, > > Here's a 3rd revision of this proposed patch: > > http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.03/ > > I used LinkedHashMap for annotations in this one. It means that now even > .getAnnotations() are reported in "declaration order": 1st inherited > (includes overridden) then declared (that are not overriding). For example, > using @Inherited annotations A1, A2, A3: > > @A1("C") > @A2("C") > class C {} > > @A1("D") > @A3("D") > class D extends C {} > > D.class.getAnnotations() now returns: { @A1("D"), @A2("C"), @A3("D") } ... > > The LHM constructor uses the following expression to estimate the initial > capacity of the LHM: > > 3326 annotations = new LinkedHashMap<>((Math.max( > 3327 declaredAnnotations.size(), > 3328 Math.min(12, declaredAnnotations.size() > + superAnnotations.size()) > 3329 ) * 4 + 2) / 3 > 3330 ); > > > I think this strikes some balance between effectivity and accuracy of > estimation. I could pre-scan the superclass annotations and calculate the > exact final size of the annotations Map before constructing it though. Tell > me if this is worth the effort. > > I also created a test that tests 3 things: > - class annotation inheritance > - order of class annotations reported by .getAnnotations() and > .getDeclaredAnnotations() methods (although not specified, the order is now > stable and resembles declaration order) > - behaviour of class annotation caching when class is redefined > > > Regards, Peter > > On 08/13/2013 09:52 AM, Joel Borggrén-Franck wrote: >> Hi, >> >> Comments inline, >> >> On 12 aug 2013, at 14:40, Peter Levart <peter.lev...@gmail.com> wrote: >>> On 08/12/2013 12:54 PM, Joel Borggren-Franck wrote: >>> - 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. >> Thanks. >> >>> 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? >> My bad. I was thinking of the case where every inherited annotation was >> overridden, in that case annotations.size() == declaredAnnotations.size(). >> That is of course not generally true. I'm fine with handling this as a >> follow up since the situation is no worse than today and the surrounding >> code is better. However, >> >> 1) We should really measure this. >> 2) My gut feeling is that the ratio of not overridden inherited annotations >> to declared annotations is small IE the expected nr of annotations is much >> closer to declare annotations than to declared + superclass annotations. >> 3) The default initial capacity is 16 and load factor is 0.75. I do think >> the mean nr of annotations is closer to 3 than to 12. We are probably >> wasting some space here. >> >> Perhaps use min(default cap, (total annotations/0.75 (+ 1?))) for now as >> that will make the situation no worse than today and often better? >> >>>> 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. >> I'm a bit worried these tests doesn't cover annotations + class redefine. >> But I might be persuaded to have more extensive testing as a follow up as >> well. >> >> cheers >> /Joel >
