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

[Maurizio Cimadamore]

On Tue, 1 Feb 2022 17:42:21 GMT, Jorn Vernee <jvernee 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
>
> Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Address review comments
>  - Change ValueLayout toString

Marked as reviewed by mcimadamore (Committer).

src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 662:

> 660:     }
> 661: 
> 662:     private void emitInvokeStatic(String ownerInternalName, String methodName, String descriptor) {

Any reason as to why using a String for the owner instead of Class<?> - you want to avoid multiple computations, I guess? Having a class literal in the clients is much more readable IMHO than having a string constant.

That said, maybe we can find a compromise :-)

Why not naming the INTRN constants in a regular way, so that it's clear what they refer to?
e.g.

JAVA_LANG_INVOKE_MEMORYLAYOUT

or, if we are ok dropping the package, just:


MEMORY_LAYOUT


E.g. drop the INTRN suffix - that should make things more readable.

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

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