Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v4]
On Tue, 15 Feb 2022 19:59:43 GMT, Mandy Chung wrote: >> This patch removes the restriction in the raw library loading mechanism that >> does not allow mix-n-match of loading a library as a JNI library and as a >> raw library. >> >> The raw library loading mechanism is designed for panama to load native >> library essentially equivalent to dlopen/dlclose calls independent of JNI >> library loading. If a native library is loaded as a JNI library and a raw >> library, it will get different NativeLibrary instances. When a class loader >> is being unloaded, JNI_Unload will be invoked but the native library may not >> be unloaded until NativeLibrary::unload is explicitly called for the raw >> library. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > No need for explicit locking for raw loading Looks good! Thanks for the changes! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v3]
On Tue, 15 Feb 2022 19:59:11 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line
>> 427:
>>
>>> 425: new ConcurrentHashMap<>();
>>> 426:
>>> 427: static void acquireNativeLibraryLock(String libraryName) {
>>
>> Note sure about these two routines. Should they be shared? Should we have
>> two locks, one for JNI, one for raw? Of course, as is the code looks fine -
>> but I wonder if a single lock isn't overly strict.
>
> I initially planned to come back to this but I obviously forgot.
>
> This does not need the per-library locking for JNI library which avoids the
> deadlock due to the class and library loading scenarios involving JNI_OnLoad,
> class initialization, and `Sytem::loadLibrary`. I now removed the locking
> entirely as it uses the concurrent hash map to register/unregister the loaded
> libraries. Much simplified.
Thanks again for the feedback. This version is much cleaner.
-
PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v3]
On Tue, 15 Feb 2022 09:48:21 GMT, Maurizio Cimadamore
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove the coupling of RawNativeLibraries with JNI library loading
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 427:
>
>> 425: new ConcurrentHashMap<>();
>> 426:
>> 427: static void acquireNativeLibraryLock(String libraryName) {
>
> Note sure about these two routines. Should they be shared? Should we have two
> locks, one for JNI, one for raw? Of course, as is the code looks fine - but I
> wonder if a single lock isn't overly strict.
I initially planned to come back to this but I obviously forgot.
This does not need the per-library locking for JNI library which avoids the
deadlock due to the class and library loading scenarios involving JNI_OnLoad,
class initialization, and `Sytem::loadLibrary`. I now removed the locking
entirely as it uses the concurrent hash map to register/unregister the loaded
libraries. Much simplified.
> src/java.base/share/classes/jdk/internal/loader/RawNativeLibraries.java line
> 107:
>
>> 105: * {@link System#mapLibraryName} can be used to convert a name to
>> 106: * a platform-specific pathname:
>> 107: * {@snipplet
>
> Suggestion:
>
> * {@snippet
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v4]
> This patch removes the restriction in the raw library loading mechanism that > does not allow mix-n-match of loading a library as a JNI library and as a raw > library. > > The raw library loading mechanism is designed for panama to load native > library essentially equivalent to dlopen/dlclose calls independent of JNI > library loading. If a native library is loaded as a JNI library and a raw > library, it will get different NativeLibrary instances. When a class loader > is being unloaded, JNI_Unload will be invoked but the native library may not > be unloaded until NativeLibrary::unload is explicitly called for the raw > library. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: No need for explicit locking for raw loading - Changes: - all: https://git.openjdk.java.net/jdk/pull/7435/files - new: https://git.openjdk.java.net/jdk/pull/7435/files/755c67d9..1be1afad Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7435=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7435=02-03 Stats: 45 lines in 2 files changed: 14 ins; 18 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/7435.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7435/head:pull/7435 PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v3]
On Tue, 15 Feb 2022 02:17:00 GMT, Mandy Chung wrote:
>> This patch removes the restriction in the raw library loading mechanism that
>> does not allow mix-n-match of loading a library as a JNI library and as a
>> raw library.
>>
>> The raw library loading mechanism is designed for panama to load native
>> library essentially equivalent to dlopen/dlclose calls independent of JNI
>> library loading. If a native library is loaded as a JNI library and a raw
>> library, it will get different NativeLibrary instances. When a class loader
>> is being unloaded, JNI_Unload will be invoked but the native library may not
>> be unloaded until NativeLibrary::unload is explicitly called for the raw
>> library.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove the coupling of RawNativeLibraries with JNI library loading
Looks - good, I like the way this patch makes a cleaner distinction between
JNI-like and more dlopen-like library loading.
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 427:
> 425: new ConcurrentHashMap<>();
> 426:
> 427: static void acquireNativeLibraryLock(String libraryName) {
Note sure about these two routines. Should they be shared? Should we have two
locks, one for JNI, one for raw? Of course, as is the code looks fine - but I
wonder if a single lock isn't overly strict.
src/java.base/share/classes/jdk/internal/loader/RawNativeLibraries.java line
107:
> 105: * {@link System#mapLibraryName} can be used to convert a name to
> 106: * a platform-specific pathname:
> 107: * {@snipplet
Suggestion:
* {@snippet
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v3]
> This patch removes the restriction in the raw library loading mechanism that > does not allow mix-n-match of loading a library as a JNI library and as a raw > library. > > The raw library loading mechanism is designed for panama to load native > library essentially equivalent to dlopen/dlclose calls independent of JNI > library loading. If a native library is loaded as a JNI library and a raw > library, it will get different NativeLibrary instances. When a class loader > is being unloaded, JNI_Unload will be invoked but the native library may not > be unloaded until NativeLibrary::unload is explicitly called for the raw > library. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Remove the coupling of RawNativeLibraries with JNI library loading - Changes: - all: https://git.openjdk.java.net/jdk/pull/7435/files - new: https://git.openjdk.java.net/jdk/pull/7435/files/6e492d2b..755c67d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7435=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7435=01-02 Stats: 505 lines in 7 files changed: 259 ins; 213 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/7435.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7435/head:pull/7435 PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 10:11:50 GMT, Maurizio Cimadamore wrote: > there is still residual common logic in how clients are expected to load > libraries (e.g. either using a file name/absolute path, or using a library > name, without file separator chars). These assumption seem very heavily > influenced by how System::load/loadLibrary do things, I believe - and I > wonder if they can, in the future, create other obstacles for a raw kind of > library loading (which expects to be mostly a wrapper around > dlopen/LoadLibrary). But that's a question/problem for another day. Good point. With the removal of the restriction, the raw library loading implementation can further be cleaned up (not be tied with the jni library loading logic). I'll look into what it takes and whether I should do it in this PR. - PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 03:49:45 GMT, Mandy Chung wrote: >> This patch removes the restriction in the raw library loading mechanism that >> does not allow mix-n-match of loading a library as a JNI library and as a >> raw library. >> >> The raw library loading mechanism is designed for panama to load native >> library essentially equivalent to dlopen/dlclose calls independent of JNI >> library loading. If a native library is loaded as a JNI library and a raw >> library, it will get different NativeLibrary instances. When a class loader >> is being unloaded, JNI_Unload will be invoked but the native library may not >> be unloaded until NativeLibrary::unload is explicitly called for the raw >> library. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review comment Looks good. I like the fact that the factory for raw library now takes a MH.lookup instead of a class. And I also like that the paths for loading a library in raw mode vs. JNI mode are more clearly separated, I think this should help Panama, thanks! One fly-by comment is that there is still residual common logic in how clients are expected to load libraries (e.g. either using a file name/absolute path, or using a library name, without file separator chars). These assumption seem very heavily influenced by how System::load/loadLibrary do things, I believe - and I wonder if they can, in the future, create other obstacles for a raw kind of library loading (which expects to be mostly a wrapper around dlopen/LoadLibrary). But that's a question/problem for another day. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 03:49:45 GMT, Mandy Chung wrote: >> This patch removes the restriction in the raw library loading mechanism that >> does not allow mix-n-match of loading a library as a JNI library and as a >> raw library. >> >> The raw library loading mechanism is designed for panama to load native >> library essentially equivalent to dlopen/dlclose calls independent of JNI >> library loading. If a native library is loaded as a JNI library and a raw >> library, it will get different NativeLibrary instances. When a class loader >> is being unloaded, JNI_Unload will be invoked but the native library may not >> be unloaded until NativeLibrary::unload is explicitly called for the raw >> library. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review comment Marked as reviewed by sundar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
> This patch removes the restriction in the raw library loading mechanism that > does not allow mix-n-match of loading a library as a JNI library and as a raw > library. > > The raw library loading mechanism is designed for panama to load native > library essentially equivalent to dlopen/dlclose calls independent of JNI > library loading. If a native library is loaded as a JNI library and a raw > library, it will get different NativeLibrary instances. When a class loader > is being unloaded, JNI_Unload will be invoked but the native library may not > be unloaded until NativeLibrary::unload is explicitly called for the raw > library. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/7435/files - new: https://git.openjdk.java.net/jdk/pull/7435/files/bbb44b6b..6e492d2b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7435=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7435=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7435.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7435/head:pull/7435 PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library [v2]
On Fri, 11 Feb 2022 02:38:08 GMT, Athijegannathan Sundararajan
wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comment
>
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 58:
>
>> 56: * will fail.
>> 57: */
>> 58: public abstract class NativeLibraries {
>
> could this be sealed with only two specific subtypes defined here?
It could. It's just an internal class and not so much of a concern of
unexpected subclasses. I'll leave it as is.
> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 251:
>
>> 249: // do not search java.library.path
>> 250: super(loader, loader != null ? null : NativeLibraries.class,
>> 251: loader != null ? true : false);
>
> The last argument expression of the super class could be just loader != null
Updated. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7435
Re: RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library
On Thu, 10 Feb 2022 23:27:49 GMT, Mandy Chung wrote:
> This patch removes the restriction in the raw library loading mechanism that
> does not allow mix-n-match of loading a library as a JNI library and as a raw
> library.
>
> The raw library loading mechanism is designed for panama to load native
> library essentially equivalent to dlopen/dlclose calls independent of JNI
> library loading. If a native library is loaded as a JNI library and a raw
> library, it will get different NativeLibrary instances. When a class loader
> is being unloaded, JNI_Unload will be invoked but the native library may not
> be unloaded until NativeLibrary::unload is explicitly called for the raw
> library.
LGTM
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 58:
> 56: * will fail.
> 57: */
> 58: public abstract class NativeLibraries {
could this be sealed with only two specific subtypes defined here?
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line 251:
> 249: // do not search java.library.path
> 250: super(loader, loader != null ? null : NativeLibraries.class,
> 251: loader != null ? true : false);
The last argument expression of the super class could be just loader != null
-
Marked as reviewed by sundar (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7435
RFR: 8281335: Allow a library already loaded via System::loadLibrary to be loaded as a raw library
This patch removes the restriction in the raw library loading mechanism that does not allow mix-n-match of loading a library as a JNI library and as a raw library. The raw library loading mechanism is designed for panama to load native library essentially equivalent to dlopen/dlclose calls independent of JNI library loading. If a native library is loaded as a JNI library and a raw library, it will get different NativeLibrary instances. When a class loader is being unloaded, JNI_Unload will be invoked but the native library may not be unloaded until NativeLibrary::unload is explicitly called for the raw library. - Commit messages: - Allow raw libraries to be loaded even if it's loaded via System::loadLibrary Changes: https://git.openjdk.java.net/jdk/pull/7435/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7435=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281335 Stats: 324 lines in 4 files changed: 191 ins; 115 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/7435.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7435/head:pull/7435 PR: https://git.openjdk.java.net/jdk/pull/7435
