[foreign-abi] RFR: 8247937: Specialize downcall binding recipes using MethodHandle combinators

[Maurizio Cimadamore]

On Fri, 19 Jun 2020 16:10:36 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Hi,
> 
> This PR adds specialization of downcall binding recipes using MethodHandles. The main addition to the code is the
> ProgrammableInvoker::specialize method, but there are also some refactorings which I will explain;
> The current code has a single `invoke` method that takes the high-level input arguments of the down call as an
> Object[]. This method calls the BindingInterpreter to pre-process arguments, and also move any low-level values
> produced by the pre-processing directly into the intermediate argument buffer that is later passed to the native code
> that copies the values into registers and then calls the target function.  This `invoke` method is split into 2;
> `invokeInterpBindings`, which pre-processes the arguments and outputs any low-level values produced by the
> pre-processing into an Object[].  This Object[] is then passed to `invokeMoves`, which moves the low-level values into
> the intermediate argument buffer that is passed to native.  This split is done so that we can replace the
> invokeInterpBindings call with a specialized MethodHandle chain based on the binding recipe instead, which is built on
> top a type handle of invokeMoves.  This takes care of the binding recipe specialization. invokeMoves can later be
> replaced during C2 compilation with code that passes these low-level values into registers directly (but that is for
> another patch).  BindingInterpreter now doesn't write values to the intermediate buffer directly, and so instead of
> passing functors to obtain a pointer to write a low-level value to, it now takes functors that handle the reading or
> writing (see StoreFunc and LoadFunc).  Since the read/write methods in BindingInterpreter are now called from multiple
> places, I've moved them to SharedUtils.
> ---
> 
> The process of specializing the binding recipe is as follows:
> 
> We first calculate a low-level method type (the 'intrinsicType') based on the MOVE bindings of a particular recipe,
> this gives us a method type that takes only primitives as arguments, which represent the values to be copied into the
> various CPU registers before calling the native target function. We then get a low-level MethodHandle that calls
> invokeMoves, and adapt it to the intrinsic type. On top of that we build the specialized binding recipe method handle
> chain.  For each argument, we iterate through the bindings in reverse, and for each binding insert a new filter MH onto
> the handle we already have. At the end we will end up with a handle that features the high-level arguments of our
> native function (MemorySegment, MemoryAddress, etc.).  The same is done for the return bindings; the return value is
> repeatedly filtered according to the bindings.  This currently doesn't work for multiple return values, in which case
> invokeMoves will return an Object[] instead of just a plain Object. This case is not specialized with method handles,
> but is instead handled solely by invokeInterpBindings. (we detect this in getBoundMethodHandle)  In case the bindings
> need to do allocation, a NativeAllocationScope is created and passed in as the first argument, which is then forwarded
> to the various filter MH using mergeArguments (which is a wrapper for MethodHandles::permuteArguments that merges 2
> arguments given their indices). The allocation and cleanup of the NativeAllocationScope are handled with a tryFinally
> combinator.
> ---
> 
> I've added a system property to disable the MethodHandle specialization so that the performance difference can be
> measured. The speed up I've seen on the CallOverhead benchmark between this and the current code is about 1.5x (but
> with the C2 intrinsics this gets us on par with JNI). The difference between specialization turned off and on is a
> little bigger than that, since the split of the 'invoke' method adds a bit of overhead as well, but it's still on
> overall gain compared to what we currently have.  Finally, I've also fixed a minor problems with TestUpcall where the
> expected and actual values were reversed when calling asserts.
> ---
> 
> If I've missed anything in the explanation, please feel free to ask!
> 
> Thanks,
> Jorn

Overall looks good, I've left a couple of minor editorial comments.

The architectural approach, while sound, to seems a bit risky. We're essentially adding yet another way to interpret
bindings, and one that is less scrutable (since interpreting happens implicitly, via a MH chain) which will add cost in
terms of maintenance going forwards. The speedup, seems good, but at the same time I can't help thinking that the basic
binding interpreter has not been optimized much, so we don't really know how much of that performance gap is really due
to the fact that interpreting bindings using a chain of MH is faster (I can see advantages in _not_ allocating the
binding array, but other than that things look less obvious).

As a code organization strategy, I also wonder if it would pay off to bring more API to bindings, e.g. so that each
binding could support a box/unbox/specialize triad of methods - rather than having switches scattered in several places?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 388:

> 387:                                 Map<VMStorage, Integer> retIndexMap) throws Throwable {
> 388:         List<MemorySegment> tempBuffers = new ArrayList<>();
> 389:         try {

Should this also be expressed in terms of NativeScope?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 186:

> 185:
> 186:     private MethodHandle specialize(MethodHandle intrinsicHandle) {
> 187:         MethodType type = callingSequence.methodType();

I think some renaming of variables would be helpful here - e.g. the parameter is a movesHandle, or a leafHandle - then
we can use some other name, like specializedHandle for the thing we're building - intrinsics is a bit vague and is used
to refer to two very different handles.

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

Marked as reviewed by mcimadamore (Committer).

PR: https://git.openjdk.java.net/panama-foreign/pull/212