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

[Maurizio Cimadamore]

On Sat, 21 Aug 2021 22:37:05 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> This reimplements core reflection with method handles.
>> 
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation uses `MethodHandle`.  For `Field` accessor, the new implementation uses `VarHandle`.    For the first few invocations of one of these reflective methods on a specific reflective object we invoke the corresponding method handle directly. After that we spin a dynamic bytecode stub defined in a hidden class which loads the target `MethodHandle` or `VarHandle` from its class data as a dynamically computed constant. Loading the method handle from a constant allows JIT to inline the method-handle invocation in order to achieve good performance.
>> 
>> The VM's native reflection methods are needed during early startup, before the method-handle mechanism is initialized. That happens soon after System::initPhase1 and before System::initPhase2, after which we switch to using method handles exclusively.
>> 
>> The core reflection and method handle implementation are updated to handle chained caller-sensitive method calls [1] properly. A caller-sensitive method can define with a caller-sensitive adapter method that will take an additional caller class parameter and the adapter method will be annotated with `@CallerSensitiveAdapter` for better auditing.   See the detailed description from [2].
>> 
>> Ran tier1-tier8 tests.   
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2] https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove separate accessor for static vs instance method
>   
>   There is no effective difference when using MethodHandle::dropArgument for static method.   Removing Static*Accessor and Instance*Accessor simplifies the implementation.

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.

 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.

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.

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

src/java.base/share/classes/jdk/internal/reflect/DirectConstructorAccessorImpl.java line 106:

> 104:     Object invokeImpl(Object[] args) throws Throwable {
> 105:         var mhInvoker = mhInvoker();
> 106:         return switch (paramCount) {

As @plevart observed, I'm also a bit surprised to see this, but I note your comments regarding performance - especially in cold case. Every adaptation counts, I guess, if you're not in the hot path. But let's make sure that the pay off to add the extra hand-specialized cases is worth it - I'd be surprised if spreading arguments is that expensive.

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

src/java.base/share/classes/jdk/internal/reflect/VarHandleBooleanFieldAccessorImpl.java line 32:

> 30: import java.lang.reflect.Modifier;
> 31: 
> 32: abstract class VarHandleBooleanFieldAccessorImpl extends VarHandleFieldAccessorImpl {

I wondered if we could avoid these hand specialized versions of VH accessors (one for each carrier type) by instead getting the getter/setter method handle out of the var handle, and adapt that so that its type matches Object... but then I realized that the `Field` API has sharp types in the getter/setters for primitives, so this seems like a forced move (maybe to revisit with Valhalla) :-(.

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

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