Looks good Peter, cheers /Joel
On 2013-09-15, Peter Levart wrote: > Hi, > > I rebased the changes and added @bug tag to test. Here's new webrev: > > http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.04/ > > > Joel, I believe you've got the "R" mojo now... > > > Regards, Peter > > On 09/09/2013 06:57 PM, Peter Levart wrote: > >Hi Joel, > > > >Thanks for reviewing. > > > >On 09/09/2013 04:25 PM, Joel Borggrén-Franck wrote: > >>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. > > > >I'll do that. I just didn't know whether tagging with bug-ID is > >meant for all tests that arise from fixing a particular bug or > >just those that test the presence/fixture of the bug. The 8011940 > >bug is about scalability and the test is not testing scalability > >(it has been demonstrated by a micro-benchmark, but that is not > >included in the test). The test is just covering the logic that > >has been re-factored and has not had any tests before. > > > >Regards, Peter > > > >>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 > > >
