On Tue, 17 May 2022 08:41:59 GMT, Maurizio Cimadamore <[email protected]>
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
