[foreign-preview] RFR: 8278414: Replace binding recipe customization using MH combinators with bytecode spinning

[Jorn Vernee]

On Tue, 1 Feb 2022 14:48:09 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch removes the binding recipe specialization using MethodHandle combinators in the linker implementation, and replaces it with specialization by spinning a class using the ASM bytecode API.
>> 
>> The main reason for doing this is simplicity of the implementation, as well as finer-grained control over the classes that are generated.
>> 
>> The meat of this PR is in `jdk.internal.foreign.abi.Specializer`, but there are some spin-off changes in this PR as well:
>> - MethodHandles are really good at sharing classes, so to get comparable performance, this PR also adds a cache behind CLinker::downcallHandle. Due to this, I've also created an AbstractLinker base class for all linker implementations, which contains the cache as well as other shared code.
>> - CallingSequence is used to steer the class generation. I've added some predicates to this class to check whether the CS is for downcall or upcall. As well as splitting the methodType() accessor into callerMethodType() and calleeMethodType(), reflecting the 2 method types in play during generation (one of the 2 was being computed ad-hoc during specialization previously). This is also responsible for most of the test changes. I've added some javadoc to CS which hopefully adequately explains these accessors.
>> - I've slightly changed the toString output of ValueLayout to include carrier types as well, in order to be able to use FunctionDescriptor.toString() as a file name in case we want to dump the generated classes to disc. This avoids conflicts between e.g. long and double, which previously were both stringified as b64.
>> 
>> Thanks,
>> Jorn
>
> src/java.base/share/classes/jdk/internal/foreign/abi/Specializer.java line 260:
> 
>> 258:         if (callingSequence.allocationSize() != 0) {
>> 259:             emitConst(callingSequence.allocationSize());
>> 260:             mv.visitMethodInsn(INVOKESTATIC, BINDING_CONTEXT_INTRN, OF_BOUNDED_ALLOCATOR_NAME, OF_BOUNDED_ALLOCATOR_DESC, false);
> 
> In this, and other instructions, I wonder if it would be more readable to have helper routine, and rely on class literal/strings more e.g. instead of:
> 
>             mv.visitMethodInsn(INVOKESTATIC, BINDING_CONTEXT_INTRN, OF_BOUNDED_ALLOCATOR_NAME, OF_BOUNDED_ALLOCATOR_DESC, false);
> 
> something like:
> 
> 
> emitInvokeStatic(Binding.Context.class, "ofBoundedAllocator", OF_BOUNDED_ALLOCATOR_DESC);

That's a good idea. I think we can use DirectMethodHandleDesc to describe the method we want to call

> src/java.base/share/classes/jdk/internal/foreign/abi/Specializer.java line 399:
> 
>> 397:         for (Binding binding : bindings) {
>> 398:             switch (binding.tag()) {
>> 399:                 case VM_STORE -> emitVMStore((Binding.VMStore) binding);
> 
> I note that we depart from the previous model of having virtual calls on the Binding instance which does the right thing. That's fine, I guess there's a lot of coupling (esp. in VMLoad/VMStore) with the specializer state.

Yeah, there's a dependency on the specializer state. But, I also personally like that the code is not interspersed with other code like class definitions and code for interpretation/verification, like in the Binding classes. I find that it reads a little bit easier.

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

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