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

[Peter Levart]

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
>>>