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