RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v3]

[Mandy Chung]

On Fri, 27 Aug 2021 13:28:08 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Overall, seems like a solid piece of work. I did not review in full the intricacies with caller sensitive (as I don't know that area too much), but the general rewrite seems solid.

Thanks for the review.

> One thing I had trouble with was the naming of the various method accessors - for instance, DirectMethodAccessorImpl vs. NativeMethodAccessorImpl vs. DelegatingMethodAccessorImpl vs. AdaptiveMethodAccessor (and there's more). It's not always easy to grasp from the method name which one is new and which one is old. Maybe sprinkling the `Handle` word on any accessor impl would help a bit with that.

I can see how this could be confusing.   Sprinkling the `Handle` word may help.  I will try that.

> Similarly, I found the `ClassByteBuilder` name a bit weak, since that is meant to only create instance of custom MHInvokers. A more apt name will help there too.

What about `InvokerBuilder` (it creates either MHInvoker or VHInvoker)?

> I also had some issues parsing the flow in `ReflectionFactory::newMethodAccessor` (and constructor, has similar issue):
> 
> ```
>            if (useNativeAccessor(method)) {
>                 return DirectMethodAccessorImpl.nativeAccessor(method, callerSensitive);
>             }
>             return MethodHandleAccessorFactory.newMethodAccessor(method, callerSensitive);
> ```
> 
> Why can't logic like this be encapsulated in a single factory call? E.g. it seems like the code is would like to abstract the differences between the various accessor implementations, but it doesn't seem to get all the way there (it's also possible I'm missing some constraint here).

We have to avoid to initialize the `MethodHandleAccessorFactory` class until `java.lang.invoke` is initialized. because `MethodHandleAccessorFactory::<clinit>` creates lookup object and  method handles.   I agree it's cleaner to encapsulate these in one single factory method.    I think moving the the static fields and the initialization to a holder class may be a way to do it.  I will give it a try.

> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 200:
> 
>> 198:             return MethodHandleAccessorFactory.newMethodAccessor(method, callerSensitive);
>> 199:         } else {
>> 200:             if (!useDirectMethodHandle && noInflation
> 
> seems to me that the `!useDirectMethodHandle` is useless here (always false) ?

Good catch!  Will fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/5027