RFR: 8029019: (ann) Optimize annotation handling in core reflection

[Claes Redestad]

Hi Peter,

thanks for the thorough examination of this issue. After going through
the patch I do like what I see, but I have a few comments:

AnnotationInvocationHandler:
in invoke, cleaning up and replacing the explicit AssertionError should
be fine, but perhaps convert it to an assert that the number of
arguments is 1 when methodName is "equals" and 0 otherwise?

Adding @Stable is fine if that has a performance impact, but I think
we might preserve clarity of intent if you kept the fields as final and
kept using Unsafe to deserialize properly.

AnnotationSupport:
I dislike that this code was already catching Throwable, and that
you're now copying that approach.  Could the new the catch clause mimic
the one that previously wrapped the entire method?

AnnotationType:
accessibleMembers might not need to be volatile.

validateAnnotationMethod:
the block: label and break block seems unnecessary. I'd not worry
about optimizing for such invalid cases and simply run through all the
checks and set valid = false multiple times if so be it.

Thanks!

/Claes

On 2017-01-17 15:10, Peter Levart wrote:
> Hi,
>
> This is a preview of a patch that addresses the following issue:
>
>     https://bugs.openjdk.java.net/browse/JDK-8029019
>
> But it is also an attempt to clean-up failure reporting for invalid
> annotation types. Here's the 1st webrev:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/8029019_Annotations.optimization/webrev.01/
>
>
> With this patch applied, running the following benchmark:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/8029019_Annotations.optimization/AnnotationsBench.java
>
>
> Produces the following results:
>
> Original:
>
> Benchmark                                 Mode  Cnt Score     Error  Units
> AnnotationsBench.a1_PrimitiveMember       avgt   10 20.790 ±   1.317  ns/op
> AnnotationsBench.a2_ObjectMember          avgt   10 21.550 ±   0.580  ns/op
> AnnotationsBench.a3_PrimitiveArrayMember  avgt   10 28.314 ±   1.477  ns/op
> AnnotationsBench.a4_ObjectArrayMember     avgt   10 27.094 ±   0.574  ns/op
> AnnotationsBench.a5_AnnotationType        avgt   10 13.047 ±   0.983  ns/op
> AnnotationsBench.a6_HashCode              avgt   10 158.001 ±   9.347
> ns/op
> AnnotationsBench.a7_Equals                avgt   10 663.921 ±  27.256
> ns/op
> AnnotationsBench.a8_ToString              avgt   10 1772.901 ± 107.355
> ns/op
>
> Patched:
>
> Benchmark                                 Mode  Cnt Score    Error  Units
> AnnotationsBench.a1_PrimitiveMember       avgt   10 8.547 ±  0.100  ns/op
> AnnotationsBench.a2_ObjectMember          avgt   10 8.352 ±  0.340  ns/op
> AnnotationsBench.a3_PrimitiveArrayMember  avgt   10 17.526 ±  0.696  ns/op
> AnnotationsBench.a4_ObjectArrayMember     avgt   10 15.639 ±  0.116  ns/op
> AnnotationsBench.a5_AnnotationType        avgt   10 4.692 ±  0.154  ns/op
> AnnotationsBench.a6_HashCode              avgt   10 142.954 ±  7.460  ns/op
> AnnotationsBench.a7_Equals                avgt   10 184.535 ±  0.917  ns/op
> AnnotationsBench.a8_ToString              avgt   10 1806.685 ± 96.370
> ns/op
>
>
> Allocation rates are also reduced:
>
> Original:
>
> AnnotationsBench.a1_PrimitiveMember:·gc.alloc.rate.norm avgt   10
> 16.000 ±   0.001    B/op
> AnnotationsBench.a2_ObjectMember:·gc.alloc.rate.norm avgt   10    16.000
> ±   0.001    B/op
> AnnotationsBench.a3_PrimitiveArrayMember:·gc.alloc.rate.norm avgt
> 10    64.000 ±   0.001    B/op
> AnnotationsBench.a4_ObjectArrayMember:·gc.alloc.rate.norm avgt   10
> 48.000 ±   0.001    B/op
> AnnotationsBench.a5_AnnotationType:·gc.alloc.rate.norm avgt   10
> 16.000 ±   0.001    B/op
> AnnotationsBench.a6_HashCode:·gc.alloc.rate.norm avgt   10   128.000 ±
> 0.001    B/op
> AnnotationsBench.a7_Equals:·gc.alloc.rate.norm avgt   10   120.001 ±
> 0.001    B/op
> AnnotationsBench.a8_ToString:·gc.alloc.rate.norm avgt   10  5200.002 ±
> 0.001    B/op
>
> Patched:
>
> AnnotationsBench.a1_PrimitiveMember:·gc.alloc.rate.norm avgt   10    ≈
> 10⁻⁵              B/op
> AnnotationsBench.a2_ObjectMember:·gc.alloc.rate.norm avgt   10    ≈
> 10⁻⁵              B/op
> AnnotationsBench.a3_PrimitiveArrayMember:·gc.alloc.rate.norm avgt
> 10    48.000 ±   0.001    B/op
> AnnotationsBench.a4_ObjectArrayMember:·gc.alloc.rate.norm avgt   10
> 32.000 ±   0.001    B/op
> AnnotationsBench.a5_AnnotationType:·gc.alloc.rate.norm avgt   10    ≈
> 10⁻⁵              B/op
> AnnotationsBench.a6_HashCode:·gc.alloc.rate.norm avgt   10    64.000 ±
> 0.001    B/op
> AnnotationsBench.a7_Equals:·gc.alloc.rate.norm avgt   10    24.000 ±
> 0.001    B/op
> AnnotationsBench.a8_ToString:·gc.alloc.rate.norm avgt   10  5136.002 ±
> 0.001    B/op
>
>
> As for the failure reporting: requesting an annotation for invalid
> annotation type now always produces AnnotationFormatError. Previously,
> some invalid annotations were simply skipped, some produced
> AnnotationFormatError and for some of them, AnnotationFormatError was
> raised only when evaluating Annotation's equals() method.
>
> I would like to discuss this failure behavior more in-depth.
>
> Regards, Peter
>