Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 15:05:26 GMT, Jorn Vernee wrote:
>> It's not quite that simple since a binding recipe for a single parameter can
>> have multiple VMStores for instance if a struct is decomposed into multiple
>> values.
>>
>> It can be done by pulling the binding loops up to the `specialize` method,
>> and have if statements for VMStore and VMLoad, like this:
>>
>> for (Binding binding : callingSequence.argumentBindings(i)) {
>> if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forDowncall()) {
>> emitSetOutput(vms.type());
>> } else if (binding instanceof Binding.VMLoad &&
>> callingSequence.forUpcall()) {
>> emitGetInput();
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> And for returns:
>>
>> for (Binding binding : callingSequence.returnBindings()) {
>> if (binding instanceof Binding.VMLoad vml &&
>> callingSequence.forDowncall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitRestoreReturnValue(vml.type());
>> } else {
>> emitReturnBufferLoad(vml);
>> }
>> } else if (binding instanceof Binding.VMStore vms &&
>> callingSequence.forUpcall()) {
>> if (!callingSequence.needsReturnBuffer()) {
>> emitSaveReturnValue(vms.type());
>> } else {
>> emitReturnBufferStore(vms);
>> }
>> } else {
>> doBinding(binding);
>> }
>> }
>>
>> But, maybe that's better?
>
> I think someone might look at this and think "why aren't these bindings
> handled by `doBinding`"?
Let's leave it as is for the time being. I guess what I'm trying to get at is
to try and make the code more explicit - but that is hard, given the
constraints.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 14:51:02 GMT, Jorn Vernee wrote:
>> 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".
>
> It's not quite that simple since a binding recipe for a single parameter can
> have multiple VMStores for instance if a struct is decomposed into multiple
> values.
>
> It can be done by pulling the binding loops up to the `specialize` method,
> and have if statements for VMStore and VMLoad, like this:
>
> for (Binding binding : callingSequence.argumentBindings(i)) {
> if (binding instanceof Binding.VMStore vms &&
> callingSequence.forDowncall()) {
> emitSetOutput(vms.type());
> } else if (binding instanceof Binding.VMLoad &&
> callingSequence.forUpcall()) {
> emitGetInput();
> } else {
> doBinding(binding);
> }
> }
>
> And for returns:
>
> for (Binding binding : callingSequence.returnBindings()) {
> if (binding instanceof Binding.VMLoad vml &&
> callingSequence.forDowncall()) {
> if (!callingSequence.needsReturnBuffer()) {
> emitRestoreReturnValue(vml.type());
> } else {
> emitReturnBufferLoad(vml);
> }
> } else if (binding instanceof Binding.VMStore vms &&
> callingSequence.forUpcall()) {
> if (!callingSequence.needsReturnBuffer()) {
> emitSaveReturnValue(vms.type());
> } else {
> emitReturnBufferStore(vms);
> }
> } else {
> doBinding(binding);
> }
> }
>
> But, maybe that's better?
I think someone might look at this and think "why aren't these bindings handled
by `doBinding`"?
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 14:21:55 GMT, Maurizio Cimadamore
wrote:
>> 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".
It's not quite that simple since a binding recipe for a single parameter can
have multiple VMStores for instance if a struct is decomposed into multiple
values.
It can be done by pulling the binding loops up to the `specialize` method, and
have if statements for VMStore and VMLoad, like this:
for (Binding binding : callingSequence.argumentBindings(i)) {
if (binding instanceof Binding.VMStore vms &&
callingSequence.forDowncall()) {
emitSetOutput(vms.type());
} else if (binding instanceof Binding.VMLoad &&
callingSequence.forUpcall()) {
emitGetInput();
} else {
doBinding(binding);
}
}
And for returns:
for (Binding binding : callingSequence.returnBindings()) {
if (binding instanceof Binding.VMLoad vml &&
callingSequence.forDowncall()) {
if (!callingSequence.needsReturnBuffer()) {
emitRestoreReturnValue(vml.type());
} else {
emitReturnBufferLoad(vml);
}
} else if (binding instanceof Binding.VMStore vms &&
callingSequence.forUpcall()) {
if (!callingSequence.needsReturnBuffer()) {
emitSaveReturnValue(vms.type());
} else {
emitReturnBufferStore(vms);
}
} else {
doBinding(binding);
}
}
But, maybe that's better?
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Wed, 18 May 2022 11:23:07 GMT, Jorn Vernee 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
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 08:41:59 GMT, Maurizio Cimadamore
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
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 11:18:20 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
>> line 943:
>>
>>> 941: Z, B, S, C, I, J, F, D, L;
>>> 942:
>>> 943: static BasicType of(Class cls) {
>>
>> This seems a duplication of ASM Type class for no good reason,
>> Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper
>> variant opcode
>
> Didn't know about that. Neat!
Removed the `BasicType` enum, switched to using `Type` and `Type.getSort()`.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 06:00:37 GMT, Rémi Forax 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 157:
>
>> 155: }
>> 156:
>> 157: static MethodHandle doSpecialize(MethodHandle leafHandle,
>> CallingSequence callingSequence, ABIDescriptor abi) {
>
> I think thise method should be split in to, one version for the upcall, one
> for the downcall, i do not see the point of merging the two codes.
I've split this method into a specializeDowncall and specializeUpcall method.
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 816:
>
>> 814: return;
>> 815: }
>> 816: if (con instanceof Integer) {
>
> those can use the instanceof + type pattern
Done
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
> line 967:
>
>> 965:
>> 966: // unaligned constants
>> 967: public final static ValueLayout.OfBoolean JAVA_BOOLEAN_UNALIGNED =
>> JAVA_BOOLEAN;
>
> as far as i understand, those constants are accessed from the bytecode, they
> should be in their own class to cleanly separate the specializer part and the
> runtime part.
I've put these in a nest `Runtime` class, with a comment explaining that they
are referenced from the generated code.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 10:08:00 GMT, Jorn Vernee wrote: >> 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 > > Right, habit of writing lambda forms, where name variables are capitalized. Changed to lowercase. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 14:28:11 GMT, Rémi Forax wrote: >> What about using MethodTypeDesc/ClassDesc as building block? > > yes, it should be less expensive, the ClassDesc still need to be constructed > from Class to allow refactoring. Can't wait for constant folding :) I'll pull the `methodType(void.class).descriptorString()` into a separate constant, and reuse that. I don't think it's worth it to spend too much time here at this point though, since this code is only executed once. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 11:57:01 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java
>> line 42:
>>
>>> 40:
>>> 41: private class Node {
>>> 42: private SoftReference ref;
>>
>> this code looks like a double check locking so ref should be volatile
>
> Thanks. I thought the `moniterenter` & `monitorexit` were enough here for
> visibility. I've read up on this and it looks like `volatile` is needed to
> correctly order the constructor and apply call (and contents) before the
> write to the `ref` field.
yes, otherwise another thread than the one inside the
`monitorenter`/`monitorexit` that is initializing the object can see the object
half initialized.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 08:16:32 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java >> line 98: >> >>> 96: private static final String CLASS_DATA_DESC = >>> methodType(Object.class, MethodHandles.Lookup.class, String.class, >>> Class.class).descriptorString(); >>> 97: private static final String RELEASE0_DESC = >>> methodType(void.class).descriptorString(); >>> 98: private static final String ACQUIRE0_DESC = >>> methodType(void.class).descriptorString(); >> >> calling methodType() is quite slow because of the cache of MethodType so >> maybe those initialization should be done explicitly in a static block to >> avoid to recompute methodType(long).descriptorString() several times > > What about using MethodTypeDesc/ClassDesc as building block? yes, it should be less expensive, the ClassDesc still need to be constructed from Class to allow refactoring. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 05:51:58 GMT, Rémi Forax 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/SoftReferenceCache.java
> line 42:
>
>> 40:
>> 41: private class Node {
>> 42: private SoftReference ref;
>
> this code looks like a double check locking so ref should be volatile
Thanks. I thought the `moniterenter` & `monitorexit` were enough here for
visibility. I've read up on this and it looks like `volatile` is needed to
correctly order the constructor and apply call (and contents) before the write
to the `ref` field.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 06:13:04 GMT, Rémi Forax 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 943:
>
>> 941: Z, B, S, C, I, J, F, D, L;
>> 942:
>> 943: static BasicType of(Class cls) {
>
> This seems a duplication of ASM Type class for no good reason,
> Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper variant
> opcode
Didn't know about that. Neat!
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 08:32:54 GMT, Maurizio Cimadamore 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 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 Right, habit of writing lambda forms, where name variables are capitalized. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 05:54:39 GMT, Rémi Forax 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 98: > >> 96: private static final String CLASS_DATA_DESC = >> methodType(Object.class, MethodHandles.Lookup.class, String.class, >> Class.class).descriptorString(); >> 97: private static final String RELEASE0_DESC = >> methodType(void.class).descriptorString(); >> 98: private static final String ACQUIRE0_DESC = >> methodType(void.class).descriptorString(); > > calling methodType() is quite slow because of the cache of MethodType so > maybe those initialization should be done explicitly in a static block to > avoid to recompute methodType(long).descriptorString() several times What about using MethodTypeDesc/ClassDesc as building block? - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee 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
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee 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/BindingSpecializer.java
line 943:
> 941: Z, B, S, C, I, J, F, D, L;
> 942:
> 943: static BasicType of(Class cls) {
This seems a duplication of ASM Type class for no good reason,
Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper variant
opcode
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee 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/BindingSpecializer.java line 967: > 965: > 966: // unaligned constants > 967: public final static ValueLayout.OfBoolean JAVA_BOOLEAN_UNALIGNED = > JAVA_BOOLEAN; as far as i understand, those constants are accessed from the bytecode, they should be in their own class to cleanly separate the specializer part and the runtime part. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee 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/BindingSpecializer.java
line 157:
> 155: }
> 156:
> 157: static MethodHandle doSpecialize(MethodHandle leafHandle,
> CallingSequence callingSequence, ABIDescriptor abi) {
I think thise method should be split in to, one version for the upcall, one for
the downcall, i do not see the point of merging the two codes.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 816:
> 814: return;
> 815: }
> 816: if (con instanceof Integer) {
those can use the instanceof + type pattern
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee 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/BindingSpecializer.java
line 98:
> 96: private static final String CLASS_DATA_DESC =
> methodType(Object.class, MethodHandles.Lookup.class, String.class,
> Class.class).descriptorString();
> 97: private static final String RELEASE0_DESC =
> methodType(void.class).descriptorString();
> 98: private static final String ACQUIRE0_DESC =
> methodType(void.class).descriptorString();
calling methodType() is quite slow because of the cache of MethodType so maybe
those initialization should be done explicitly in a static block to avoid to
recompute methodType(long).descriptorString() several times
src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java
line 42:
> 40:
> 41: private class Node {
> 42: private SoftReference ref;
this code looks like a double check locking so ref should be volatile
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Thu, 12 May 2022 18:15:54 GMT, liach wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> BootstrapMethodError -> ExceptionInInitializerError
>
> test/micro/org/openjdk/bench/java/lang/foreign/LinkUpcall.java line 61:
>
>> 59: BLANK = lookup().findStatic(LinkUpcall.class, "blank",
>> MethodType.methodType(void.class));
>> 60: } catch (ReflectiveOperationException e) {
>> 61: throw new BootstrapMethodError(e);
>
> You probably mean exception in initializer error.
Change it to ExceptionInInitializerError
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8685/files - new: https://git.openjdk.java.net/jdk/pull/8685/files/80640bac..cd3daab5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8685&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8685&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8685.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8685/head:pull/8685 PR: https://git.openjdk.java.net/jdk/pull/8685
