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

[Jorn Vernee]

On Tue, 17 May 2022 08:41:59 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   BootstrapMethodError -> ExceptionInInitializerError
>
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 291:
> 
>> 289:             emitGetStatic(Binding.Context.class, "DUMMY", BINDING_CONTEXT_DESC);
>> 290:         } else {
>> 291:             emitInvokeStatic(Binding.Context.class, "ofSession", OF_SESSION_DESC);
> 
> Maybe this is micro-optimization, but I realized that in reality we probably don't need any scope/session if there are no "ToSegment" bindings.

Done

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

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

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