Review Request JDK-8228336: Refactor native library loading implementation
This patch refactors the native library loading implementation out of ClassLoader and move them to jdk.internal.loader package. This introduces a new NativeLibraries abstraction which loads and registers the loaded native libraries. Previously it was maintained in a nativeLibrary map in each ClassLoader instance and one systemNativeLibraries for the null loader case. With this change, each ClassLoader and BootLoader have its own NativeLibraries instead. NativeLibraries.java and NativeLibraries.c are mostly refactored from the existing code. Only minor cleanups are applied. This refactoring enables Panama LookupLibrary to further experiment and allow loading of a native library without the restriction that a native library can only be loaded by one class loader. The restriction is important for System::loadLibrary in loading JNI libraries that implements JNI native methods and so remain unchanged. This patch follows up the change introduced by JDK-8227587 which adds BootLoader::loadLibrary. It should belong a helper classes instead of BootLoader and this helper method is to avoid doPrivileged call if no security manager is installed for startup performance improvement. Several `privilegedXXX` methods were added for the same purpose for example sun.security.action.GetPropertyAction::privilegedGetProperty and privilegedGetProperties. So I think sun.security.action.LoadLibraryAction is a proper place. Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.00/ Thanks Mandy
Thanks Mandy, this refactoring would indeed enable Panama library loading to get rid of classloader-related restriction which are JNI-based and have little to do with the way Panama does things. I'm looking forward to be able to use something like this from my side of the fence :-) Cheers Maurizio On 09/03/2020 23:33, Mandy Chung wrote:
This refactoring enables Panama LookupLibrary to further experiment and allow loading of a native library without the restriction that a native library can only be loaded by one class loader. The restriction is important for System::loadLibrary in loading JNI libraries that implements JNI native methods and so remain unchanged.
Hi Mandy, On 10/03/2020 9:33 am, Mandy Chung wrote:
This patch refactors the native library loading implementation out of ClassLoader and move them to jdk.internal.loader package. This introduces a new NativeLibraries abstraction which loads and registers the loaded native libraries. Previously it was maintained in a nativeLibrary map in each ClassLoader instance and one systemNativeLibraries for the null loader case. With this change, each ClassLoader and BootLoader have its own NativeLibraries instead.
NativeLibraries.java and NativeLibraries.c are mostly refactored from the existing code. Only minor cleanups are applied.
This refactoring enables Panama LookupLibrary to further experiment and allow loading of a native library without the restriction that a native library can only be loaded by one class loader. The restriction is important for System::loadLibrary in loading JNI libraries that implements JNI native methods and so remain unchanged.
This patch follows up the change introduced by JDK-8227587 which adds BootLoader::loadLibrary. It should belong a helper classes instead of BootLoader and this helper method is to avoid doPrivileged call if no security manager is installed for startup performance improvement. Several `privilegedXXX` methods were added for the same purpose for example sun.security.action.GetPropertyAction::privilegedGetProperty and privilegedGetProperties. So I think sun.security.action.LoadLibraryAction is a proper place.
That's a core-libs decision but I'm not sure that's a namespace we want to increase usage of.
Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.00/
Overall refactoring looks okay to me. A couple of minor things I noticed: src/java.base/share/classes/java/lang/ClassLoader.java ! " in java.library.path: " + StaticProperty.sunBootLibraryPath()); s/java/sun.boot/ src/java.base/share/classes/java/lang/Runtime.java I don't see the point in changing load0 and loadLibrary0 to return NativeLibrary when it is unused. Is this to allow for direct use of these methods in the future? Thanks, David -----
Thanks Mandy
Hi David, On 3/9/20 7:41 PM, David Holmes wrote:
That's a core-libs decision but I'm not sure that's a namespace we want to increase usage of.
I'm open to other suggestion. This helper method avoids the call to doPrivileged when security manager is enabled and I think it's okay to add this helper method in sun.security.action package.
Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.00/
Overall refactoring looks okay to me. A couple of minor things I noticed:
src/java.base/share/classes/java/lang/ClassLoader.java
! " in java.library.path: " + StaticProperty.sunBootLibraryPath()); s/java/sun.boot/
Thanks for catching this.
src/java.base/share/classes/java/lang/Runtime.java
I don't see the point in changing load0 and loadLibrary0 to return NativeLibrary when it is unused. Is this to allow for direct use of these methods in the future?
It was leftover from my early prototype. It's not intended to change. I have fixed it. This new webrev includes the above minor fixes and a few windows/solaris specific files I missed from webrev.00: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.01/index.htm... Thanks Mandy
Hi Mandy, Updates seem fine to me. Thanks, David On 10/03/2020 2:53 pm, Mandy Chung wrote:
Hi David,
On 3/9/20 7:41 PM, David Holmes wrote:
That's a core-libs decision but I'm not sure that's a namespace we want to increase usage of.
I'm open to other suggestion. This helper method avoids the call to doPrivileged when security manager is enabled and I think it's okay to add this helper method in sun.security.action package.
Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.00/
Overall refactoring looks okay to me. A couple of minor things I noticed:
src/java.base/share/classes/java/lang/ClassLoader.java
! " in java.library.path: " + StaticProperty.sunBootLibraryPath()); s/java/sun.boot/
Thanks for catching this.
src/java.base/share/classes/java/lang/Runtime.java
I don't see the point in changing load0 and loadLibrary0 to return NativeLibrary when it is unused. Is this to allow for direct use of these methods in the future?
It was leftover from my early prototype. It's not intended to change. I have fixed it.
This new webrev includes the above minor fixes and a few windows/solaris specific files I missed from webrev.00: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.01/index.htm...
Thanks Mandy
On 10/03/2020 04:53, Mandy Chung wrote:
Hi David,
On 3/9/20 7:41 PM, David Holmes wrote:
That's a core-libs decision but I'm not sure that's a namespace we want to increase usage of.
I'm open to other suggestion. This helper method avoids the call to doPrivileged when security manager is enabled and I think it's okay to add this helper method in sun.security.action package. BootLoader defines methods to correspond to the methods defined by ClassLoader so I don't think the loadLibrary method was too bad. Moreover it was very clear when calling BootLoader.loadLibrary that it would attempt to load it from the system path. I don't object to putting this in sun.security.action but it doesn't feel. If it is moved but I think we'll need to find better names so it's clear in the 20+ places where it used (and also avoid the temptation to use it in the modules that aren't mapped to the boot loader).
-Alan
Hi Mandy, On 11/03/2020 16:10, Alan Bateman wrote:
I'm open to other suggestion. This helper method avoids the call to doPrivileged when security manager is enabled and I think it's okay to add this helper method in sun.security.action package. BootLoader defines methods to correspond to the methods defined by ClassLoader so I don't think the loadLibrary method was too bad. Moreover it was very clear when calling BootLoader.loadLibrary that it would attempt to load it from the system path. I don't object to putting this in sun.security.action but it doesn't feel. If it is moved but I think we'll need to find better names so it's clear in the 20+ places where it used (and also avoid the temptation to use it in the modules that aren't mapped to the boot loader).
That's probably a stupid question but wouldn't it have been simpler to redefine BootLoader::loadLibrary(String) to call the new LoadLibraryAction rather than removing the method altogether? best regards, -- daniel
-Alan
On 3/11/20 10:25 AM, Daniel Fuchs wrote:
Hi Mandy,
On 11/03/2020 16:10, Alan Bateman wrote:
I'm open to other suggestion. This helper method avoids the call to doPrivileged when security manager is enabled and I think it's okay to add this helper method in sun.security.action package. BootLoader defines methods to correspond to the methods defined by ClassLoader so I don't think the loadLibrary method was too bad. Moreover it was very clear when calling BootLoader.loadLibrary that it would attempt to load it from the system path. I don't object to putting this in sun.security.action but it doesn't feel. If it is moved but I think we'll need to find better names so it's clear in the 20+ places where it used (and also avoid the temptation to use it in the modules that aren't mapped to the boot loader).
That's probably a stupid question but wouldn't it have been simpler to redefine BootLoader::loadLibrary(String) to call the new LoadLibraryAction rather than removing the method altogether?
Thanks for the feedback. I discussed further with Alan. Other class and names are no better than BootLoader::loadLibrary. We agree to keep that helper method as BootLoader::loadLibrary. The changes due to the renaming is reverted. Updated patch: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.02/ Mandy
On 11/03/2020 21:43, Mandy Chung wrote:
:
I discussed further with Alan. Other class and names are no better than BootLoader::loadLibrary. We agree to keep that helper method as BootLoader::loadLibrary. The changes due to the renaming is reverted.
Updated patch: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8228336/webrev.02/ This version also renames a method in NativeLibrary so the name is consistent.
Looks good to me. -Alan
participants (5)
-
Alan Bateman
-
Daniel Fuchs
-
David Holmes
-
Mandy Chung
-
Maurizio Cimadamore