RFR: 8029019: (ann) Optimize annotation handling in core reflection
Peter Levart
peter.levart at gmail.com
Wed Jan 25 19:01:10 UTC 2017
Hi Claes,
On 01/24/2017 03:34 PM, Claes Redestad wrote:
> Hi Peter,
>
> thanks for the thorough examination of this issue.
Thanks for looking into this change...
> 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?
In the following part of code:
public Object invoke(Object proxy, Method method, Object[] args) {
String memberName = method.getName(); // guaranteed interned String
Class<?> dtype = method.getDeclaringClass();
if (dtype == Object.class || // equals/hashCode/toString
dtype == Annotation.class // annotationType
) {
if (memberName == "equals") return equalsImpl(proxy, args[0]);
if (memberName == "hashCode") return hashCodeImpl();
if (memberName == "annotationType") return type;
if (memberName == "toString") return toStringImpl();
throw new AssertionError("Invalid method: " + method);
}
...in the if statement, when method's declaring class is either Object
or Annotation, we have just a limited number of methods that are
possible candidates and we can identify them just by their names
(Method::getName returns interned string, so identity comparison is
possible). For example: if method's name is "equals" and declaring class
is either Object or Annotation, then we know that this method has a
signature of Object::equals - we don't need to check - this is part of
language spec.
The "throw new AssertionError("Invalid method: " + method)" is an
unreachable statement until some new method is added to either Object or
Annotation and at that time, if it ever comes to that, this code should
fail and should be revised...
The assert you are talking about is meaningful only if inserted as 1st
statement inside the if statement. Will add it.
>
> 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.
Ok, will make them final but keep @Stable.
>
> 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?
I think we can do it even better than that. See new webrev...
>
> AnnotationType:
> accessibleMembers might not need to be volatile.
AnnotationType.accessibleMembers() may be called concurrently from
multiple threads (it is used by annotation's equals method when passed a
foreign annotation and by AnnotationSupport.getValueArray() helper
method used to access repeatable annotations in a foreign container
annotation). What this method returns is a HashMap object which is not
tolerable to unsafe publication (unlike String, for example). Volatile
field is needed for safe publication of the HashMap instance.
>
> 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.
I changed the method to a boolean-returning isValidAnnotationMethod()
with multiple exit paths and moved throwing of AnnotationFormatError
into AnnotationType constructor.
Here's all that applied together with comment from Chris:
http://cr.openjdk.java.net/~plevart/jdk9-dev/8029019_Annotations.optimization/webrev.02/
plus more:
AnnotationInvocationHandler.equalsImpl()
- should check the number of values in both annotations and not the
lengths of hash tables (hash table lengths are rounded to power of 2)
- exception handling when a foreign annotation throws an unchecked
exception from it's member method - propagate it instead of returning
"false" from equals
AnnotationInvocationHandler.asOneOfUs()
- enclose the Proxy.getInvocationHandler() in doPrivileged() if needed
otherwise we can get SecurityException
AnnotationInvocationHandler.readObject()
- don't modify the Map instance read from stream as it might be shared
or unmodifiable - clone it and modify it if necessary.
AnnotationParser:
- skip annotations for types that are not annotation types any more and
propagate AnnotationFormatError for invalid annotation types.
AnnotationSupport.getValueArray()
- exception handling cleanup.
I just noticed this last thing will need another webrev and some
discussion as handling of "oneOfUs" vs. "foreign" annotation is
different currently. Should a RuntimeException (other than
IncompleteAnnotationException) be propagated or wrapped with
AnnotationFormatError? RuntimeException(s) are exceptions thrown by
various ExceptionProxy(s) when retrieving annotation values that are
somehow invalid.
Here's the diff between webrev.01 and 02 (without changes to the test):
http://cr.openjdk.java.net/~plevart/jdk9-dev/8029019_Annotations.optimization/webrev.01to02/
Regards, Peter
>
> 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
>>
More information about the core-libs-dev
mailing list