RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]

[Maurizio Cimadamore]

On Wed, 18 May 2022 11:23:07 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 336:
>> 
>>> 334: 
>>> 335:             if (callingSequence.forUpcall()) {
>>> 336:                 // for upcalls, recipes have a result, which we handle here
>> 
>> I find this part a bit confusing. The comment speaks about recipes input and outputs, hinting at the fact that downcall bindings have inputs, while upcall bindings have outputs.
>> In reality, if we look at the bindings themselves, they look pretty symmetric:
>> 
>> * UNBOX_ADDRESS = pops an Addressable and push a long on the stack
>> * BOX_ADDRESS = pops a long and push a MemoryAddress on the stack
>> 
>> That is, in both cases it appears we have an input and an output (and the binding is just the function which maps one into another).
>> 
>> I think the input/output asymmetry comes in when we consider the full recipes. In downcalls, for a given argument we will have the following sequence:
>> 
>> Binding0, Binding1, ... BindingN-1, VMStore
>> 
>> While for upcalls we will have:
>> 
>> VMLoad, Binding1, Binding2, ... BindingN
>> 
>> So (ignoring return buffer for simplicity), for downcalls, it is obvious that before we can execute Binding0, we need some kind of "input value" (e.g. the value of some local). For upcalls, this is not needed, as the VMLoad binding will basically do that for free.
>> When we finished executing the bindings, again, for downcalls there's nothing to do, as the chain always ends with a VMStore (which stores binding result into some local), while for upcalls we do need to set the output value explicitly.
>> 
>> In other words, it seems to me that having VMLoad and VMStore in the chains of bindings to be interpreted/specialized does more harm than good here. These low level bindings make sense for the VM code, as the VM needs to know how to load a value from a register, or how to store a value into a register. But in terms of the specializing Java code, these bindings are immaterial. At the same time, the fact that we have these bindings, and that they are virtualized, makes the code hard to follow, as some preparation happens in `BindingSpecializer::specialize`, while some other preparation happens inside `BindingSpecializer::doBindings`, as part of the virtualized impl of VMLoad/VMStore. And, this virtualized implementation ends up doing similar steps as what the preparation code before/after the binding execution does (e.g. for downcalls/VMStore we call setOutput, while for upcalls/VMLoad we call getInput).
>> 
>> All I'm saying here is that, for bindings to work, we _always_ need to call both getInput and setOutput - but it is easy to overlook this when looking at the code, because the getInput/setOutput calls are spread across two different places in the specializing code, perhaps making things more asymmetric than they need to be. (I realize I'm simplifying here, and that some details of the return buffer are not 100% quite as symmetric).
>
> I'm not sure if there is anything actionable here?
> 
> I've thought in the past that it might be nice to have `GetArgument`/`SetArgument` and `GetReturnValue`/`SetReturnValue` binding operators as well, to make the inputs/outputs more explicit in the recipe. But, it doesn't seem like that would make things _much_ better...

I wasn't suggesting to add more bindings. I was more suggesting to filter out the load/store from the set of bindings (since these are virtualized anyways) that are passed to `doBindings`. Then, before executing a set of bindings, (if we are in downcall mode) we load the corresponding input local var. After executing bindings (if we are in upcall mode) we store result in corresponding var.

E.g. make the logic that load locals and store locals explicit in the `specialize` method, rather than have parts of it execute "in  disguise" as "binding interpretation".

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

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