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
> 

Reply via email to