Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Mon, 19 Oct 2020 11:24:45 GMT, Jorn Vernee wrote: >> I looked through some Hotspot runtime code and that looks ok. I saw a >> couple of strange things on my way through the code. See comments. > > Hi David, this code somewhat predates me, so I initially kept the JVM_ENTRY > since that was what was already in place. IIRC the thread state transition > was added later to be able to call JNI code, which checks that the thread > state is native in some asserts. > > I've re-written this code, per @coleenp 's suggestion, to use VM code > directly to replace what we were doing with JNI, so the thread state > transition is also gone. > > I've looked at some of the *_ENTRY macros and the only one that seems to > avoid the thread state transition is JVM_LEAF. I've switched the > RegisterNatives functions we use to JVM_LEAF to avoid the redundant > transitions. I also tried changing `PI_invokeNative` to JVM_LEAF, but since > we can call back into Java from that, I run into a missing handle mark assert > for some of the tests, so I've left that one as JVM_ENTRY (but removed some > redundant braces). > > I've created a separate sub-pr against this PR's branch to make it easier to > see what I've changed: https://github.com/mcimadamore/jdk/pull/1 (feel free > to take a look). > > Thanks for the comments. I've fixed the following issues from review comments: - don't rely on `MethodHandles::adapter_code_size` (after private discussion) - update copyright years - use VM-internal API instead of JNI when parsing ABIDescriptor and BufferLayout objects while generating down/up call wrappers. As far as I see, that covers all open review comments. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Fri, 16 Oct 2020 11:12:01 GMT, Jorn Vernee wrote:
>> src/hotspot/cpu/x86/foreign_globals_x86.cpp line 56:
>>
>>> 54: }
>>> 55:
>>> 56: const ABIDescriptor parseABIDescriptor(JNIEnv* env, jobject jabi) {
>>
>> I don't know if you care about performance but of these env->calls
>> transition into the VM and back out again. You
>> should prefix all the code that comes from java to native with JNI_ENTRY and
>> just use native JVM code to implement
>> these.
>
> Currently this is prefixed with `JVM_ENTRY` e.g. like:
> JVM_ENTRY(jlong, PI_generateAdapter(JNIEnv* env, jclass _unused, jobject abi,
> jobject layout))
> {
> ThreadToNativeFromVM ttnfvm(thread);
> return ProgrammableInvoker::generate_adapter(env, abi, layout);
> }
> JVM_END
> (where `generate_adapter` ends up calling `parseABIDescriptor`)
>
> JVM_ENTYRY seems to be mostly the same except for JNI_ENTRY having a
> `WeakPreserverExceptionMark` as well. Do we need
> to switch these? Also, I guess if we want to use VM code directly, we should
> get rid of the `ThreadToNativeFromVM` RAII
> handle.
re-wrote this code to use the VM internal APIs instead of JNI, changes are
isolated in a sub-pr here:
https://github.com/mcimadamore/jdk/pull/1 Could you take a look?
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 23:15:07 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > I looked through some Hotspot runtime code and that looks ok. I saw a couple > of strange things on my way through the > code. See comments. Hi David, this code somewhat predates me, so I initially kept the JVM_ENTRY since that was what was already in place. IIRC the thread state transition was added later to be able to call JNI code, which checks that the thread state is native in some asserts. I've re-written this code, per @coleenp 's suggestion, to use VM code directly to replace what we were doing with JNI, so the thread state transition is also gone. I've looked at some of the *_ENTRY macros and the only one that seems to avoid the thread state transition is JVM_LEAF. I've switched the RegisterNatives functions we use to JVM_LEAF to avoid the redundant transitions. I also tried changing `PI_invokeNative` to JVM_LEAF, but since we can call back into Java from that, I run into a missing handle mark assert for some of the tests, so I've left that one as JVM_ENTRY (but removed some redundant braces). I've created a separate sub-pr against this PR's branch to make it easier to see what I've changed: https://github.com/mcimadamore/jdk/pull/1 (feel free to take a look). Thanks for the comments. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
Hi Jorn,
I'm not reviewing this but this exchange caught my attention ...
On 16/10/2020 9:15 pm, Jorn Vernee wrote:
On Thu, 15 Oct 2020 22:42:49 GMT, Coleen Phillimore wrote:
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Re-add file erroneously deleted (detected as rename)
src/hotspot/cpu/x86/foreign_globals_x86.cpp line 56:
54: }
55:
56: const ABIDescriptor parseABIDescriptor(JNIEnv* env, jobject jabi) {
I don't know if you care about performance but of these env->calls transition
into the VM and back out again. You
should prefix all the code that comes from java to native with JNI_ENTRY and
just use native JVM code to implement
these.
Currently this is prefixed with `JVM_ENTRY` e.g. like:
JVM_ENTRY(jlong, PI_generateAdapter(JNIEnv* env, jclass _unused, jobject abi,
jobject layout))
{
ThreadToNativeFromVM ttnfvm(thread);
return ProgrammableInvoker::generate_adapter(env, abi, layout);
}
JVM_END
(where `generate_adapter` ends up calling `parseABIDescriptor`)
JVM_ENTYRY seems to be mostly the same except for JNI_ENTRY having a
`WeakPreserverExceptionMark` as well. Do we need
to switch these? Also, I guess if we want to use VM code directly, we should
get rid of the `ThreadToNativeFromVM` RAII
handle.
Why are you going from native to VM to native again with this code? You
would use a JNI/JVM_ENTRY because you have to execute VM runtime code.
But your code immediately switches back to native and doesn't execute
any VM runtime code (other than that involved in the transition logic
itself). ??
Cheers,
David
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:42:49 GMT, Coleen Phillimore wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Re-add file erroneously deleted (detected as rename)
>
> src/hotspot/cpu/x86/foreign_globals_x86.cpp line 56:
>
>> 54: }
>> 55:
>> 56: const ABIDescriptor parseABIDescriptor(JNIEnv* env, jobject jabi) {
>
> I don't know if you care about performance but of these env->calls transition
> into the VM and back out again. You
> should prefix all the code that comes from java to native with JNI_ENTRY and
> just use native JVM code to implement
> these.
Currently this is prefixed with `JVM_ENTRY` e.g. like:
JVM_ENTRY(jlong, PI_generateAdapter(JNIEnv* env, jclass _unused, jobject abi,
jobject layout))
{
ThreadToNativeFromVM ttnfvm(thread);
return ProgrammableInvoker::generate_adapter(env, abi, layout);
}
JVM_END
(where `generate_adapter` ends up calling `parseABIDescriptor`)
JVM_ENTYRY seems to be mostly the same except for JNI_ENTRY having a
`WeakPreserverExceptionMark` as well. Do we need
to switch these? Also, I guess if we want to use VM code directly, we should
get rid of the `ThreadToNativeFromVM` RAII
handle.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:52:14 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 3885: > >> 3883: >> 3884: __ flush(); >> 3885: } > > I think as a future RFE we should refactor this function and > generate_native_wrapper since they're similar (this is > nicer to read). If I can remove is_critical_native code they will be more > similar. Yes, I've had similar thoughts as well. This is meant to be temporary code any ways. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:44:54 GMT, Coleen Phillimore wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Re-add file erroneously deleted (detected as rename)
>
> src/hotspot/cpu/x86/foreign_globals_x86.hpp line 32:
>
>> 30: #define __ _masm->
>> 31:
>> 32: struct VectorRegister {
>
> Why are these structs and not classes?
The fields are meant to be accessed directly, so I went with `struct` since the
members default to public.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:39:50 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > src/hotspot/cpu/x86/foreign_globals_x86.cpp line 2: > >> 1: /* >> 2: * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. > > Copyright should be 2020. All the new files should have 2020 as the > copyright, a bunch don't. Ok, will go and check them. FWIW, this file was added back in 2018 in the panama repo. But, I suppose it is considered new here? - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 17:08:28 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the first incubation round >> of the foreign linker access API incubation >> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory >> access support (see JEP 393 [2] and >> associated pull request [3]). >> The main goal of this API is to provide a way to call native functions from >> Java code without the need of intermediate >> JNI glue code. In order to do this, native calls are modeled through the >> MethodHandle API. I suggest reading the >> writeup [4] I put together few weeks ago, which illustrates what the foreign >> linker support is, and how it should be >> used by clients. Disclaimer: the pull request mechanism isn't great at >> managing *dependent* reviews. For this reasons, >> I'm attaching a webrev which contains only the differences between this PR >> and the memory access PR. I will be >> periodically uploading new webrevs, as new iterations come out, to try and >> make the life of reviewers as simple as >> possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the >> main architects of all the hotspot changes you >> see here, and without their help, the foreign linker support wouldn't be >> what it is today. As usual, a big thank to >> Paul Sandoz, who provided many insights (often by trying the bits first >> hand). Thanks Maurizio >> Webrev: >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev >> >> Javadoc: >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html >> >> Specdiff (relative to [3]): >> >> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254232 >> >> >> >> ### API Changes >> >> The API changes are actually rather slim: >> >> * `LibraryLookup` >> * This class allows clients to lookup symbols in native libraries; the >> interface is fairly simple; you can load a library >> by name, or absolute path, and then lookup symbols on that library. >> * `FunctionDescriptor` >> * This is an abstraction that is very similar, in spirit, to `MethodType`; >> it is, at its core, an aggregate of memory >> layouts for the function arguments/return type. A function descriptor is >> used to describe the signature of a native >> function. >> * `CLinker` >> * This is the real star of the show. A `CLinker` has two main methods: >> `downcallHandle` and `upcallStub`; the first takes >> a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a >> `FunctionDescriptor` and returns a >> `MethodHandle` instance which can be used to call the target native >> symbol. The second takes an existing method handle, >> and a `FunctionDescriptor` and returns a new `MemorySegment` >> corresponding to a code stub allocated by the VM which >> acts as a trampoline from native code to the user-provided method >> handle. This is very useful for implementing upcalls. >>* This class also contains the various layout constants that should be >> used by clients when describing native signatures >> (e.g. `C_LONG` and friends); these layouts contain additional ABI >> classfication information (in the form of layout >> attributes) which is used by the runtime to *infer* how Java arguments >> should be shuffled for the native call to take >> place. >> * Finally, this class provides some helper functions e.g. so that clients >> can convert Java strings into C strings and >> back. >> * `NativeScope` >> * This is an helper class which allows clients to group together logically >> related allocations; that is, rather than >> allocating separate memory segments using separate *try-with-resource* >> constructs, a `NativeScope` allows clients to >> use a _single_ block, and allocate all the required segments there. This >> is not only an usability boost, but also a >> performance boost, since not all allocation requests will be turned into >> `malloc` calls. >> * `MemorySegment` >> * Only one method added here - namely `handoff(NativeScope)` which allows >> a segment to be transferred onto an existing >> native scope. >> >> ### Safety >> >> The foreign linker API is intrinsically unsafe; many things can go wrong >> when requesting a native method handle. For >> instance, the description of the native signature might be wrong (e.g. have >> too many arguments) - and the runtime has, >> in the general case, no way to detect such mismatches. For these reasons, >> obtaining a `CLinker` instance is >> a *restricted* operation, which can be enabled by specifying the usual JDK >> property `-Dforeign.restricted=permit` (as >> it's the case for other restricted method in the foreign memory API). ### >> Implementation changes The Java changes >> associated with `LibraryLookup` are relative straightforward;
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
> This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-Dforeign.restricted=permit` (as > it's the case for other restricted method in the foreign memory API). ### > Implementation changes The Java changes > associated with `LibraryLookup` are relative straightforward; the only > interesting thing to note here is that library > loading does _not_ depend on class loaders, so `LibraryLookup` is not subject > to the same restriction
