Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 20/03/2020 16:17, Mandy Chung wrote: : I like these shorter names. Updated webrev.02 in place. Looks good.
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
Looks good! Thanks! Maurizio On 20/03/2020 16:17, Mandy Chung wrote: On 3/20/20 6:51 AM, Alan Bateman wrote: On 20/03/2020 03:43, Mandy Chung wrote: Alan, Maurizio, New webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.02/ newJavaNativeInterfaceLibraries creates a NativeLIbraries for loading JNI native libraries. - native libraries are unloaded when the class loader is reclaimed. - Support of linking of native method as specified in the JNI spec. - Restriction on a native library that can only be loaded by one class loader. newRawNativeLibraries creates a raw NativeLibraries for loading non-JNI native libraries. - non-JNI native library. So JNI_OnLoad and JNI_OnUnload will be ignored. No support for linking of native method. - Native libraries not auto-unloaded. They may be explicitly unloaded via NativeLibraries::unload. - No relationship with class loaders. The test is updated to show that JNI_OnLoad and JNI_OnUnload are ignored. "raw" seems okay for now, its internal so can easily be changed if there is a better name or changed further if finer control on the behavior is needed. If you are looking for a shorter name for the factory methods then maybe jniNativeLibraries and rawNativeLibraries would be better. I like these shorter names. Updated webrev.02 in place. Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 3/20/20 6:51 AM, Alan Bateman wrote: On 20/03/2020 03:43, Mandy Chung wrote: Alan, Maurizio, New webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.02/ newJavaNativeInterfaceLibraries creates a NativeLIbraries for loading JNI native libraries. - native libraries are unloaded when the class loader is reclaimed. - Support of linking of native method as specified in the JNI spec. - Restriction on a native library that can only be loaded by one class loader. newRawNativeLibraries creates a raw NativeLibraries for loading non-JNI native libraries. - non-JNI native library. So JNI_OnLoad and JNI_OnUnload will be ignored. No support for linking of native method. - Native libraries not auto-unloaded. They may be explicitly unloaded via NativeLibraries::unload. - No relationship with class loaders. The test is updated to show that JNI_OnLoad and JNI_OnUnload are ignored. "raw" seems okay for now, its internal so can easily be changed if there is a better name or changed further if finer control on the behavior is needed. If you are looking for a shorter name for the factory methods then maybe jniNativeLibraries and rawNativeLibraries would be better. I like these shorter names. Updated webrev.02 in place. Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 20/03/2020 03:43, Mandy Chung wrote: Alan, Maurizio, New webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.02/ newJavaNativeInterfaceLibraries creates a NativeLIbraries for loading JNI native libraries. - native libraries are unloaded when the class loader is reclaimed. - Support of linking of native method as specified in the JNI spec. - Restriction on a native library that can only be loaded by one class loader. newRawNativeLibraries creates a raw NativeLibraries for loading non-JNI native libraries. - non-JNI native library. So JNI_OnLoad and JNI_OnUnload will be ignored. No support for linking of native method. - Native libraries not auto-unloaded. They may be explicitly unloaded via NativeLibraries::unload. - No relationship with class loaders. The test is updated to show that JNI_OnLoad and JNI_OnUnload are ignored. "raw" seems okay for now, its internal so can easily be changed if there is a better name or changed further if finer control on the behavior is needed. If you are looking for a shorter name for the factory methods then maybe jniNativeLibraries and rawNativeLibraries would be better. -Alan
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
Alan, Maurizio, New webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.02/ newJavaNativeInterfaceLibraries creates a NativeLIbraries for loading JNI native libraries. - native libraries are unloaded when the class loader is reclaimed. - Support of linking of native method as specified in the JNI spec. - Restriction on a native library that can only be loaded by one class loader. newRawNativeLibraries creates a raw NativeLibraries for loading non-JNI native libraries. - non-JNI native library. So JNI_OnLoad and JNI_OnUnload will be ignored. No support for linking of native method. - Native libraries not auto-unloaded. They may be explicitly unloaded via NativeLibraries::unload. - No relationship with class loaders. The test is updated to show that JNI_OnLoad and JNI_OnUnload are ignored. thanks Mandy On 3/18/20 5:39 PM, Mandy Chung wrote: On 3/18/20 12:13 PM, Maurizio Cimadamore wrote: On 18/03/2020 18:40, Mandy Chung wrote: On 3/18/20 11:16 AM, Maurizio Cimadamore wrote: So, maybe I'm saying something naive, but isn't the difference between the two mechanisms mostly there to distinguish between JNI libraries and non-JNI libraries? I think such distinction is kind of blurry at the moment. One thing for sure is that JNI native method binding won't happen with the native libraries loaded through this new mechanism. OTOH, should JNI_OnLoad and JNI_Unload be invoked if it is a non-JNI library? The new mechanism still does. I expect that this will be cleared up from panama specification. Should defo not happen in Panama-loaded libraries OK, I will make some adjustment to ignore JNI_OnLoad and JNI_Unload. I think this is a clean distinction of these two mechanisms. Will send an updated webrev later. Or are there cases where you envision more mix and match? E.g. JNI libraries w/o auto-unloading? No as unloading is important for native library loaded by custom loaders. I can't really think of a good static factory method name. would newNonJavaNativeLIbraries be slightly clearer? newJavaNativeInterfaceLibraries vs. newRawNativeLibraries Both are good to me. could be an option. Another option, in case we do care about mix and match, would be to use a builder - which would allow us to specify whether we want: * auto-unloading * classloader restrictions * calling JNI hooks * support linking of JNI methods But I don't think we need such level of granularity for now. I don't think we need that neither. Mandy Maurizio Mandy Maurizio On 18/03/2020 16:32, Mandy Chung wrote: On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 3/18/20 12:13 PM, Maurizio Cimadamore wrote: On 18/03/2020 18:40, Mandy Chung wrote: On 3/18/20 11:16 AM, Maurizio Cimadamore wrote: So, maybe I'm saying something naive, but isn't the difference between the two mechanisms mostly there to distinguish between JNI libraries and non-JNI libraries? I think such distinction is kind of blurry at the moment. One thing for sure is that JNI native method binding won't happen with the native libraries loaded through this new mechanism. OTOH, should JNI_OnLoad and JNI_Unload be invoked if it is a non-JNI library? The new mechanism still does. I expect that this will be cleared up from panama specification. Should defo not happen in Panama-loaded libraries OK, I will make some adjustment to ignore JNI_OnLoad and JNI_Unload. I think this is a clean distinction of these two mechanisms. Will send an updated webrev later. Or are there cases where you envision more mix and match? E.g. JNI libraries w/o auto-unloading? No as unloading is important for native library loaded by custom loaders. I can't really think of a good static factory method name. would newNonJavaNativeLIbraries be slightly clearer? newJavaNativeInterfaceLibraries vs. newRawNativeLibraries Both are good to me. could be an option. Another option, in case we do care about mix and match, would be to use a builder - which would allow us to specify whether we want: * auto-unloading * classloader restrictions * calling JNI hooks * support linking of JNI methods But I don't think we need such level of granularity for now. I don't think we need that neither. Mandy Maurizio Mandy Maurizio On 18/03/2020 16:32, Mandy Chung wrote: On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 18/03/2020 18:40, Mandy Chung wrote: On 3/18/20 11:16 AM, Maurizio Cimadamore wrote: So, maybe I'm saying something naive, but isn't the difference between the two mechanisms mostly there to distinguish between JNI libraries and non-JNI libraries? I think such distinction is kind of blurry at the moment. One thing for sure is that JNI native method binding won't happen with the native libraries loaded through this new mechanism. OTOH, should JNI_OnLoad and JNI_Unload be invoked if it is a non-JNI library? The new mechanism still does. I expect that this will be cleared up from panama specification. Should defo not happen in Panama-loaded libraries However, you raise a good point that this is not only about auto-unloading (which I got trapped as this patch is all about). No JNI native method binding is another significant part. Yes E.g. maybe we should add JNI somewhere in one of the factory (the one used by System.loadLibrary) and then document what are the differences between JNI and non-JNI libraries. We could even have different NativeLibrary impl for these two cases. Seems to me that JNI libs feature: * extra restrictions (cannot load same library in multiple loaders) * auto-unloading guarantees (classloader-driven) JNI native method binding will lookup methods from these libraries. Yeah that too Or are there cases where you envision more mix and match? E.g. JNI libraries w/o auto-unloading? No as unloading is important for native library loaded by custom loaders. I can't really think of a good static factory method name. would newNonJavaNativeLIbraries be slightly clearer? newJavaNativeInterfaceLibraries vs. newRawNativeLibraries could be an option. Another option, in case we do care about mix and match, would be to use a builder - which would allow us to specify whether we want: * auto-unloading * classloader restrictions * calling JNI hooks * support linking of JNI methods But I don't think we need such level of granularity for now. Maurizio Mandy Maurizio On 18/03/2020 16:32, Mandy Chung wrote: On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 3/18/20 11:16 AM, Maurizio Cimadamore wrote: So, maybe I'm saying something naive, but isn't the difference between the two mechanisms mostly there to distinguish between JNI libraries and non-JNI libraries? I think such distinction is kind of blurry at the moment. One thing for sure is that JNI native method binding won't happen with the native libraries loaded through this new mechanism. OTOH, should JNI_OnLoad and JNI_Unload be invoked if it is a non-JNI library? The new mechanism still does. I expect that this will be cleared up from panama specification. However, you raise a good point that this is not only about auto-unloading (which I got trapped as this patch is all about). No JNI native method binding is another significant part. E.g. maybe we should add JNI somewhere in one of the factory (the one used by System.loadLibrary) and then document what are the differences between JNI and non-JNI libraries. We could even have different NativeLibrary impl for these two cases. Seems to me that JNI libs feature: * extra restrictions (cannot load same library in multiple loaders) * auto-unloading guarantees (classloader-driven) JNI native method binding will lookup methods from these libraries. Or are there cases where you envision more mix and match? E.g. JNI libraries w/o auto-unloading? No as unloading is important for native library loaded by custom loaders. I can't really think of a good static factory method name. would newNonJavaNativeLIbraries be slightly clearer? Mandy Maurizio On 18/03/2020 16:32, Mandy Chung wrote: On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
So, maybe I'm saying something naive, but isn't the difference between the two mechanisms mostly there to distinguish between JNI libraries and non-JNI libraries? E.g. maybe we should add JNI somewhere in one of the factory (the one used by System.loadLibrary) and then document what are the differences between JNI and non-JNI libraries. We could even have different NativeLibrary impl for these two cases. Seems to me that JNI libs feature: * extra restrictions (cannot load same library in multiple loaders) * auto-unloading guarantees (classloader-driven) Or are there cases where you envision more mix and match? E.g. JNI libraries w/o auto-unloading? Maurizio On 18/03/2020 16:32, Mandy Chung wrote: On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 3/18/20 8:59 AM, Alan Bateman wrote: On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. "no auto unload" is also important. what about "newTrustedNativeLibrariesNoAutoUnload" a bit long? Mandy
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 17/03/2020 23:09, Mandy Chung wrote: I have similar comment to myself and didn't come up good static factory method names. I give it a try again: what about newNativeLibraries and newNativeLibrariesWithNoAutoUnload? Would newTrustedNativeLibraries work? Everything else in the updated webrev looks good. -Alan
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
Hi Alan,
Thanks for the comment. See my comments inlined below.
Here is the updated webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.01
On 3/16/20 3:47 AM, Alan Bateman wrote:
The difference between the 2 constructors might not be obvious at the
use sites. I'm just wondering if would be better to use static factory
methods instead, e.g. the 2-arg constructor could be replaced with a
trusted(caller, searchLibPath) method that would make it a lot more
obvious in the places where that will eventually be used.
I have similar comment to myself and didn't come up good static factory
method names. I give it a try again: what about newNativeLibraries and
newNativeLibrariesWithNoAutoUnload?
A small inconsistency is that VM.isSystemDomainLoader is used in
constructor whereas the other checks for null and platform class
loader (plus app class loader).
Good catch. Revised.
The Main test could use Path.of("classes"). In setup, dir could be a
Path and also Path p = Files.createDirectories(...) would allow the
Files.move to be a bit more readable.
Adjusted.
I can't quite tell why the test is skipped with -Xcomp but maybe it's
just too slow and times out?
Removed. Cut-n-paste error from an existing test.
A small suggestion for NativeLibrariesTest is that
loadWithCustomLoader might be a better name to load p.Test with a
custom loader. Also noticed libnativeLibrariesTest.c has a 2017 date
on it.
Fixed.
thanks
Mandy
-Alan.
Re: Review Request JDK-8240975: Extend NativeLibraries to support explicit unloading
On 13/03/2020 18:16, Mandy Chung wrote:
Webrev:
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8240975/webrev.00/
This is a follow-up task for Panama to allow explicit unloading of
native library after JDK-8228336. `NativeLibraries` associated with a
class loader has the capability to auto unload native libraries when
the class loader is reclaimed. This adds a new
`NativeLibraries::unload` method that provides the ability to unload a
native library on request. This only allows `NativeLibraries` of no
relationship with a class loader where auto unloading is disabled
The difference between the 2 constructors might not be obvious at the
use sites. I'm just wondering if would be better to use static factory
methods instead, e.g. the 2-arg constructor could be replaced with a
trusted(caller, searchLibPath) method that would make it a lot more
obvious in the places where that will eventually be used.
A small inconsistency is that VM.isSystemDomainLoader is used in
constructor whereas the other checks for null and platform class loader
(plus app class loader).
The Main test could use Path.of("classes"). In setup, dir could be a
Path and also Path p = Files.createDirectories(...) would allow the
Files.move to be a bit more readable. I can't quite tell why the test is
skipped with -Xcomp but maybe it's just too slow and times out?
A small suggestion for NativeLibrariesTest is that loadWithCustomLoader
might be a better name to load p.Test with a custom loader. Also noticed
libnativeLibrariesTest.c has a 2017 date on it.
-Alan.
