Hi Chris,
On 01/24/2017 04:07 PM, Chris Hegarty wrote:
Peter,
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
This very good work. I have not done a complete review, but what
jumps out at me is that you have removed some of the validation
code and checks from AnnotationInvocationHandler ( in one place
there is an assert and a comment that “good” data is expected.
AnnotationInvocationHandler has shown up in many interesting
places, I wonder if it is wise to remove these.
I don't know to what part of code you are referring. if it is the checks
in the AnnotationInvocationHandler constructor then they have been moved
to AnnotationType constructor. AnnotationParser always
constructs/retrieves AnnotationType instance for the annotationClass
object before it attempts to construct AnnotationInvocationHandler for
such annotationClass, so annotationClass is pre-validated.
AnnotationInvocationHandler is a package-private class and its
constructor is only invoked from AnnotationParser.annotationForMap()
public factory method. This method is invoked from 3 places currently:
AnnotationParser.parseAnnotation2() which pre-validates the annotationClass
AnnotationsInheritanceOrderRedefinitionTest which is passing only javac
compiled (pre-validated) annotationClasses to it
com.sun.tools.javac.model.AnnotationProxyMaker.generateAnnotation - this
one is called from various other places so there's no guarantee that the
annotationClass passed to it is validated.
So you have a point. I will include an unconditional call to
AnnotationType.getInstance(annotationClass) in the
AnnotationInvocationHandler constructor. This will only amount to an
overhead of a volatile read when AnnotationType is already cached which
it usually will be.
Regards, Peter
-Chris.
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
