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
