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

[Claes Redestad]

Hi Peter,

On 01/25/2017 08:01 PM, Peter Levart wrote:
> 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.

Yes, thanks!

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

Good.

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

Oh, right, since we're avoiding InvocationHandler::invoke we don't have to
catch Throwable here.

Tangent: could we theoretically respecify InvocationHandler to throw 
Exception
rather than Throwable (since RuntimeException and Error are always 
implicitly
thrown)?

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

I stand corrected!

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

Good - I had jotted down a note to ask about this in an early review draft.

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

I think this all seems reasonable, but subtle behavior changes like this 
needs more scrutiny than I can provide.  I've discussed this offline 
with Joe and sadly concluded it's probably too much, too late for 9 at 
this point.

Hope you don't mind re-targetting this to JDK 10.

Thanks!

/Claes