Thanks Joel,
I will push this now.
Regards, Peter
On 09/18/2013 11:28 AM, Joel Borggrén-Franck wrote:
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
