On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> 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/Binding.java line 257:
> 255: * the context's allocator is accessed.
> 256: */
> 257: public static Context ofSession() {
Thanks for fixing this
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 131:
> 129: private int[] scopeSlots;
> 130: private int curScopeLocalIdx = -1;
> 131: private int RETURN_ALLOCATOR_IDX = -1;
These are not constants, so it is odd to see the name capitalized
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.
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).
-------------
PR: https://git.openjdk.java.net/jdk/pull/8685
