Review Request JDK-8164512: Replace ClassLoader use of finalizer with phantom reference to unload native library
This patch proposes to replace the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded. The spec for JNI_OnUnload [1] specifies that this function is called in an unknown context whereas the hotspot implementation uses the class loader being unloaded as the context, which I consider a bug. It should not load a class defined to that class loader. The proposed spec change for JNI_FindClass if called from JNI_OnUnload to use system class loader (consistent with no caller context case). Webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8187882 Please see the spec change an behavioral change in this CSR. A native library may fail to be reloaded if it is loaded immediately or soon after a class loader is GC'ed but the cleaner thread doesn't yet have the chance to handle the unloading of the native library. Likewise, there is no guarantee regarding the timing of finalization and a native library may fail to reload. It's believed that the compatibility risk should be low. I'm looking into adding a native jtreg test that will be added in the next revision. Mandy [1] http://docs.oracle.com/javase/9/docs/specs/jni/invocation.html#jni_onunload
Hi, Mandy The changes look alright to me. One thing that I noticed: ClassLoader.NativeLibrary.register() writes to 'loadedLibraryNames', 'nativeLibraries', and 'systemNativeLibraries' without first synchronizing on them. There is not a bug here per se, as register() is called from inside the needed synchronized blocks in loadLibrary0(). But perhaps it's worth a comment to call out that this is what is happening? Thanks, -Brent On 9/22/17 3:18 PM, mandy chung wrote:
This patch proposes to replace the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded.
The spec for JNI_OnUnload [1] specifies that this function is called in an unknown context whereas the hotspot implementation uses the class loader being unloaded as the context, which I consider a bug. It should not load a class defined to that class loader. The proposed spec change for JNI_FindClass if called from JNI_OnUnload to use system class loader (consistent with no caller context case).
Webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8187882
Please see the spec change an behavioral change in this CSR. A native library may fail to be reloaded if it is loaded immediately or soon after a class loader is GC'ed but the cleaner thread doesn't yet have the chance to handle the unloading of the native library. Likewise, there is no guarantee regarding the timing of finalization and a native library may fail to reload. It's believed that the compatibility risk should be low.
I'm looking into adding a native jtreg test that will be added in the next revision.
Mandy [1] http://docs.oracle.com/javase/9/docs/specs/jni/invocation.html#jni_onunload
On Sep 22, 2017, at 6:18 PM, mandy chung <mandy.chung@oracle.com> wrote:
This patch proposes to replace the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded.
The spec for JNI_OnUnload [1] specifies that this function is called in an unknown context whereas the hotspot implementation uses the class loader being unloaded as the context, which I consider a bug. It should not load a class defined to that class loader. The proposed spec change for JNI_FindClass if called from JNI_OnUnload to use system class loader (consistent with no caller context case).
Webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8187882
Please see the spec change an behavioral change in this CSR. A native library may fail to be reloaded if it is loaded immediately or soon after a class loader is GC'ed but the cleaner thread doesn't yet have the chance to handle the unloading of the native library. Likewise, there is no guarantee regarding the timing of finalization and a native library may fail to reload. It's believed that the compatibility risk should be low.
I'm looking into adding a native jtreg test that will be added in the next revision.
Mandy [1] http://docs.oracle.com/javase/9/docs/specs/jni/invocation.html#jni_onunload
Thanks for dealing with this. ============================================================================== src/java.base/share/native/libjava/ClassLoader.c 415 Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload 416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, jlong address) With this change, the "this" argument is no longer used. Because of this, the native function could be a static member function of the new Unloader class, or could (I think) still be a (now static) member function of NativeLibrary. The latter would not require a name change, only a signature change. But I don't really care which class has the method. ============================================================================== src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 } Can anything before the cleanup registration throw? If so, do we leak because we haven't registered the cleanup yet? And what if the registration itself fails to complete? ============================================================================== src/hotspot/share/prims/jni.cpp 405 // Special handling to make sure JNI_OnLoad are executed in the correct class context. s/are executed/is executed/ ==============================================================================
On 9/25/17 11:37 PM, Kim Barrett wrote:
Thanks for dealing with this.
I'm all for eliminating the finalizers in the JDK. Looking forward to having a finalizer-free JDK.
============================================================================== src/java.base/share/native/libjava/ClassLoader.c 415 Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload 416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, jlong address)
With this change, the "this" argument is no longer used.
Because of this, the native function could be a static member function of the new Unloader class, or could (I think) still be a (now static) member function of NativeLibrary. The latter would not require a name change, only a signature change. But I don't really care which class has the method. Yes it can be a static method. ============================================================================== src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 }
Can anything before the cleanup registration throw?
No within the register method. The builtin loaders are created early during startup. I shall make sure that System::loadLibrary is not called during init phase 1.
If so, do we leak because we haven't registered the cleanup yet? And what if the registration itself fails to complete?
If Cleaner::register fails, it should throw an exception. Otherwise, the registration should complete.
============================================================================== src/hotspot/share/prims/jni.cpp 405 // Special handling to make sure JNI_OnLoad are executed in the correct class context.
s/are executed/is executed/
Will fix it. I'm considering to separate the JNI_FindClass change to target 18.3 and provide a flag to restore the current behavior so that it may help existing code to identify its dependency on the current behavior and give time to migrate. Then target the finalizer to Cleaner change in 18.9. It's unknown to us how many existing native libraries are impacted by this change (calling FindClass from JNI_OnUnload to load classes visible the class loader being unloaded). I suspect it's rare. If FindClass is called when the native library is being unloaded and fail to find the class due to this change, it is not hard to find out but the code might crash if it does not handle the class not found case properly. Any opinion? Mandy
On Sep 26, 2017, at 1:27 PM, mandy chung <mandy.chung@oracle.com> wrote: On 9/25/17 11:37 PM, Kim Barrett wrote:
src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 }
Can anything before the cleanup registration throw?
No within the register method.
I think there are some opportunities for OOME, but I think no worse than before. And the result would be a loaded library without the unload registration, which seems like it might perhaps be annoying but probably not fatal.
I'm considering to separate the JNI_FindClass change to target 18.3 and provide a flag to restore the current behavior so that it may help existing code to identify its dependency on the current behavior and give time to migrate. Then target the finalizer to Cleaner change in 18.9.
It's unknown to us how many existing native libraries are impacted by this change (calling FindClass from JNI_OnUnload to load classes visible the class loader being unloaded). I suspect it's rare. If FindClass is called when the native library is being unloaded and fail to find the class due to this change, it is not hard to find out but the code might crash if it does not handle the class not found case properly.
Any opinion?
I’m in favor of removing this finalizer sooner rather than later, but you probably could have guessed that.
Hi Mandy, On 27/09/2017 3:27 AM, mandy chung wrote:
On 9/25/17 11:37 PM, Kim Barrett wrote:
Thanks for dealing with this.
I'm all for eliminating the finalizers in the JDK. Looking forward to having a finalizer-free JDK.
==============================================================================
src/java.base/share/native/libjava/ClassLoader.c 415 Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload 416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, jlong address)
With this change, the "this" argument is no longer used.
Because of this, the native function could be a static member function of the new Unloader class, or could (I think) still be a (now static) member function of NativeLibrary. The latter would not require a name change, only a signature change. But I don't really care which class has the method. Yes it can be a static method. ==============================================================================
src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 }
Can anything before the cleanup registration throw?
No within the register method. The builtin loaders are created early during startup. I shall make sure that System::loadLibrary is not called during init phase 1.
If so, do we leak because we haven't registered the cleanup yet? And what if the registration itself fails to complete?
If Cleaner::register fails, it should throw an exception. Otherwise, the registration should complete.
==============================================================================
src/hotspot/share/prims/jni.cpp 405 // Special handling to make sure JNI_OnLoad are executed in the correct class context.
s/are executed/is executed/
Will fix it.
I'm considering to separate the JNI_FindClass change to target 18.3 and provide a flag to restore the current behavior so that it may help existing code to identify its dependency on the current behavior and give time to migrate. Then target the finalizer to Cleaner change in 18.9.
It's unknown to us how many existing native libraries are impacted by this change (calling FindClass from JNI_OnUnload to load classes visible the class loader being unloaded). I suspect it's rare. If FindClass is called when the native library is being unloaded and fail to find the class due to this change, it is not hard to find out but the code might crash if it does not handle the class not found case properly.
Any opinion?
Yes :) I agree with being conservative here. We don't know how this may be being used. But I'm still not completely clear how the FindClass change is tied to the switch to Cleaner ?? Thanks, David
Mandy
On 9/26/17 5:34 PM, David Holmes wrote:
Hi Mandy,
On 27/09/2017 3:27 AM, mandy chung wrote:
On 9/25/17 11:37 PM, Kim Barrett wrote:
Thanks for dealing with this.
I'm all for eliminating the finalizers in the JDK. Looking forward to having a finalizer-free JDK.
==============================================================================
src/java.base/share/native/libjava/ClassLoader.c 415 Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload 416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, jlong address)
With this change, the "this" argument is no longer used.
Because of this, the native function could be a static member function of the new Unloader class, or could (I think) still be a (now static) member function of NativeLibrary. The latter would not require a name change, only a signature change. But I don't really care which class has the method. Yes it can be a static method. ==============================================================================
src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 }
Can anything before the cleanup registration throw?
No within the register method. The builtin loaders are created early during startup. I shall make sure that System::loadLibrary is not called during init phase 1.
If so, do we leak because we haven't registered the cleanup yet? And what if the registration itself fails to complete?
If Cleaner::register fails, it should throw an exception. Otherwise, the registration should complete.
==============================================================================
src/hotspot/share/prims/jni.cpp 405 // Special handling to make sure JNI_OnLoad are executed in the correct class context.
s/are executed/is executed/
Will fix it.
I'm considering to separate the JNI_FindClass change to target 18.3 and provide a flag to restore the current behavior so that it may help existing code to identify its dependency on the current behavior and give time to migrate. Then target the finalizer to Cleaner change in 18.9.
It's unknown to us how many existing native libraries are impacted by this change (calling FindClass from JNI_OnUnload to load classes visible the class loader being unloaded). I suspect it's rare. If FindClass is called when the native library is being unloaded and fail to find the class due to this change, it is not hard to find out but the code might crash if it does not handle the class not found case properly.
Any opinion?
Yes :) I agree with being conservative here. We don't know how this may be being used. But I'm still not completely clear how the FindClass change is tied to the switch to Cleaner ?? It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable). Is that clearer? Mandy
On 27/09/2017 11:32 AM, mandy chung wrote:
On 9/26/17 5:34 PM, David Holmes wrote:
Hi Mandy,
On 27/09/2017 3:27 AM, mandy chung wrote:
On 9/25/17 11:37 PM, Kim Barrett wrote:
Thanks for dealing with this.
I'm all for eliminating the finalizers in the JDK. Looking forward to having a finalizer-free JDK.
==============================================================================
src/java.base/share/native/libjava/ClassLoader.c 415 Java_java_lang_ClassLoader_00024NativeLibrary_00024Unloader_unload 416 (JNIEnv *env, jobject this, jstring name, jboolean isBuiltin, jlong address)
With this change, the "this" argument is no longer used.
Because of this, the native function could be a static member function of the new Unloader class, or could (I think) still be a (now static) member function of NativeLibrary. The latter would not require a name change, only a signature change. But I don't really care which class has the method. Yes it can be a static method. ==============================================================================
src/java.base/share/classes/java/lang/ClassLoader.java 2394 public void register() { [...] 2406 // register the class loader for cleanup when unloaded 2407 if (loader != getBuiltinPlatformClassLoader() && 2408 loader != getBuiltinAppClassLoader()) { 2409 CleanerFactory.cleaner() 2410 .register(loader, new Unloader(name, handle, isBuiltin)); 2411 }
Can anything before the cleanup registration throw?
No within the register method. The builtin loaders are created early during startup. I shall make sure that System::loadLibrary is not called during init phase 1.
If so, do we leak because we haven't registered the cleanup yet? And what if the registration itself fails to complete?
If Cleaner::register fails, it should throw an exception. Otherwise, the registration should complete.
==============================================================================
src/hotspot/share/prims/jni.cpp 405 // Special handling to make sure JNI_OnLoad are executed in the correct class context.
s/are executed/is executed/
Will fix it.
I'm considering to separate the JNI_FindClass change to target 18.3 and provide a flag to restore the current behavior so that it may help existing code to identify its dependency on the current behavior and give time to migrate. Then target the finalizer to Cleaner change in 18.9.
It's unknown to us how many existing native libraries are impacted by this change (calling FindClass from JNI_OnUnload to load classes visible the class loader being unloaded). I suspect it's rare. If FindClass is called when the native library is being unloaded and fail to find the class due to this change, it is not hard to find out but the code might crash if it does not handle the class not found case properly.
Any opinion?
Yes :) I agree with being conservative here. We don't know how this may be being used. But I'm still not completely clear how the FindClass change is tied to the switch to Cleaner ?? It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks?? And you want to change the FindClass spec to make it clear the test is what needs to be changed? David
Mandy
On 9/26/17 7:06 PM, David Holmes wrote:
It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks??
No. The test reveals a bug in JNI_FindClass that uses a class loader being finalized as the context when NativeLibrary is the caller.
And you want to change the FindClass spec to make it clear the test is what needs to be changed? No. It is a bug in the hotspot implementation. The JNI spec says that the context of JNI_OnUnload being called is unknown. The hotspot implementation of FindClass uses the class loader associated with that native library as the context when invoked from JNI_OnUnload which is wrong.
I will file a separate JBS issue to separate this JNI bug. Mandy
On 27/09/2017 12:11 PM, mandy chung wrote:
On 9/26/17 7:06 PM, David Holmes wrote:
It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks??
No. The test reveals a bug in JNI_FindClass that uses a class loader being finalized as the context when NativeLibrary is the caller.
And you want to change the FindClass spec to make it clear the test is what needs to be changed? No. It is a bug in the hotspot implementation. The JNI spec says that the context of JNI_OnUnload being called is unknown. The hotspot implementation of FindClass uses the class loader associated with that native library as the context when invoked from JNI_OnUnload which is wrong.
I'm not sure I agree it is wrong. As I've said elsewhere there's a good chance that if you are trying to load classes via FindClass as part of a unload hook (which implies you are using custom classloaders), then it may be only the current loader or a parent (still custom) can load that class. But we're on the fringe of realistic expectations here as the context is specified as being "unknown". That said given the spec says "unknown" the behaviour of the VM could change and still be in spec. I presume that when using a cleaner the current classloader that would be used by FindClass is the system loader? Hence the observed behaviour of FindClass "changes" if you switch to the cleaner from the finalizer - and can't be reverted to the old behaviour by using a command-line flag. Hence if we want to be able to revert we have to do that in a FindClass-only change first. Then drop-in the cleaner update and remove the flag.
I will file a separate JBS issue to separate this JNI bug.
Okay. I see this as a RFE not a bug per-se: change from "unknown context" to a specific well known context. Thanks, David
Mandy
On 9/26/17 7:35 PM, David Holmes wrote:
On 27/09/2017 12:11 PM, mandy chung wrote:
On 9/26/17 7:06 PM, David Holmes wrote:
It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks??
No. The test reveals a bug in JNI_FindClass that uses a class loader being finalized as the context when NativeLibrary is the caller.
And you want to change the FindClass spec to make it clear the test is what needs to be changed? No. It is a bug in the hotspot implementation. The JNI spec says that the context of JNI_OnUnload being called is unknown. The hotspot implementation of FindClass uses the class loader associated with that native library as the context when invoked from JNI_OnUnload which is wrong.
I'm not sure I agree it is wrong. As I've said elsewhere there's a good chance that if you are trying to load classes via FindClass as part of a unload hook (which implies you are using custom classloaders), then it may be only the current loader or a parent (still custom) can load that class. But we're on the fringe of realistic expectations here as the context is specified as being "unknown".
For a native unload hook to access some class defined by this class loader, definitely it should not write to any fields since the class and class loader are not strongly reachable. Reading the current state stored in the class can be done by writing to the native fields. I'd like to know what other use cases that FindClass must ressurrect a class defined by this class loader or find a class defined by its ancestor if you have any in mind that the existing code can't be replaced due to the proposed change.
That said given the spec says "unknown" the behaviour of the VM could change and still be in spec.
I presume that when using a cleaner the current classloader that would be used by FindClass is the system loader? Hence the observed behaviour of FindClass "changes" if you switch to the cleaner from the finalizer - and can't be reverted to the old behaviour by using a command-line flag. Hence if we want to be able to revert we have to do that in a FindClass-only change first. Then drop-in the cleaner update and remove the flag.
I will file a separate JBS issue to separate this JNI bug.
Okay. I see this as a RFE not a bug per-se: change from "unknown context" to a specific well known context. This case is arguable whether it's considered as a RFE or a bug because the current spec of JNI_OnUnload and JNI_FindClass are not aligned. I lean toward a bug. The bottom line: do you agree with this proposed JNI spec change?
Mandy
On 27/09/2017 1:36 PM, mandy chung wrote:
On 9/26/17 7:35 PM, David Holmes wrote:
On 27/09/2017 12:11 PM, mandy chung wrote:
On 9/26/17 7:06 PM, David Holmes wrote:
It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks??
No. The test reveals a bug in JNI_FindClass that uses a class loader being finalized as the context when NativeLibrary is the caller.
And you want to change the FindClass spec to make it clear the test is what needs to be changed? No. It is a bug in the hotspot implementation. The JNI spec says that the context of JNI_OnUnload being called is unknown. The hotspot implementation of FindClass uses the class loader associated with that native library as the context when invoked from JNI_OnUnload which is wrong.
I'm not sure I agree it is wrong. As I've said elsewhere there's a good chance that if you are trying to load classes via FindClass as part of a unload hook (which implies you are using custom classloaders), then it may be only the current loader or a parent (still custom) can load that class. But we're on the fringe of realistic expectations here as the context is specified as being "unknown".
For a native unload hook to access some class defined by this class loader, definitely it should not write to any fields since the class and class loader are not strongly reachable. Reading the current state stored in the class can be done by writing to the native fields.
Yes that is a good point - but as the spec says due to the unknown context the hook has to be very careful about what it tries to do. I agree it is doubtful that anyone can, or should, be relying on the direct use of the classloader that has become unreachable, but ...
I'd like to know what other use cases that FindClass must ressurrect a class defined by this class loader or find a class defined by its ancestor if you have any in mind that the existing code can't be replaced due to the proposed change.
... I can easily imagine a subsystem that runs under a custom loader and which then instantiates further execution contexts (per connection for example) each with their own classloader and which can be reclaimed after the request is complete. I can then easily imagine that they use an unload hook to record statistics about native library use, and that the statistics classes are in the top-level custom loader, and not locatable from the system loader. While the spec makes no guarantees this will work it only says programmers "should be conservative in their use of VM services" which strongly suggests to me a "try it and see if it works" approach. In the current code while loading from the loader being reclaimed is highly dubious, delegating through that loader seems fairly reasonable to me.
That said given the spec says "unknown" the behaviour of the VM could change and still be in spec.
I presume that when using a cleaner the current classloader that would be used by FindClass is the system loader? Hence the observed behaviour of FindClass "changes" if you switch to the cleaner from the finalizer - and can't be reverted to the old behaviour by using a command-line flag. Hence if we want to be able to revert we have to do that in a FindClass-only change first. Then drop-in the cleaner update and remove the flag.
I will file a separate JBS issue to separate this JNI bug.
Okay. I see this as a RFE not a bug per-se: change from "unknown context" to a specific well known context. This case is arguable whether it's considered as a RFE or a bug because the current spec of JNI_OnUnload and JNI_FindClass are not aligned. I lean toward a bug. The bottom line: do you agree with this proposed JNI spec change?
I don't think the spec _has_ to change because I disagree that there is a misalignment between JNI_OnUnload and JNI_FindClass. FindClass clearly states it uses the current loader or else the system loader if there is no notion of a current loader. OnUnload says it runs in an unknown context, so you don't know what the current loader may be, or even if there is one. But regardless a call to FindClass from OnUnload should use the current loader if it exists, or else the system loader. The fact it may be dubious to use the current loader when it is itself in the process of being unloaded does not impinge on the voracity of the spec in my opinion. So you can change to using a Cleaner instead of a finalizer and while it will behave differently, that change in behaviour does not violate the spec in any way - again in my opinion. Now if you want to pave the way for a future switch to Cleaner by changing the spec for JNI_OnUnload such that it must be executed in a context where (equivalently) there either is no current loader or the current loader is the system loader, then I do not oppose that. But the only purpose that serves is to allow a migration path to the new behaviour - and then forever locks us in. Note however I would not want to see the implementation of FindClass having to special case this - I would hope it just happens naturally if the Cleaner thread reports the current class loader as the system loader. Does it? Thanks, David
Mandy
Corrections ... On 27/09/2017 2:36 PM, David Holmes wrote:
On 27/09/2017 1:36 PM, mandy chung wrote:
On 9/26/17 7:35 PM, David Holmes wrote:
On 27/09/2017 12:11 PM, mandy chung wrote:
On 9/26/17 7:06 PM, David Holmes wrote:
It is not tied with the Cleaner change. Instead, the FindClass bug blocks the finalizer to Cleaner change.
FindClass bug is uncovered when I implemented the change from finalizer to Cleaner (or phantom reference). There is a test calling FindClass to look for a class defined by the class loader being unloaded, say L. L is not Gc'ed and so FindClass successfully finds the class (which resurrect the class loader which was marked finalizable).
Is that clearer?
So the issue is only that this test breaks??
No. The test reveals a bug in JNI_FindClass that uses a class loader being finalized as the context when NativeLibrary is the caller.
And you want to change the FindClass spec to make it clear the test is what needs to be changed? No. It is a bug in the hotspot implementation. The JNI spec says that the context of JNI_OnUnload being called is unknown. The hotspot implementation of FindClass uses the class loader associated with that native library as the context when invoked from JNI_OnUnload which is wrong.
I'm not sure I agree it is wrong. As I've said elsewhere there's a good chance that if you are trying to load classes via FindClass as part of a unload hook (which implies you are using custom classloaders), then it may be only the current loader or a parent (still custom) can load that class. But we're on the fringe of realistic expectations here as the context is specified as being "unknown".
For a native unload hook to access some class defined by this class loader, definitely it should not write to any fields since the class and class loader are not strongly reachable. Reading the current state stored in the class can be done by writing to the native fields.
Yes that is a good point - but as the spec says due to the unknown context the hook has to be very careful about what it tries to do. I agree it is doubtful that anyone can, or should, be relying on the direct use of the classloader that has become unreachable, but ...
I'd like to know what other use cases that FindClass must ressurrect a class defined by this class loader or find a class defined by its ancestor if you have any in mind that the existing code can't be replaced due to the proposed change.
... I can easily imagine a subsystem that runs under a custom loader and which then instantiates further execution contexts (per connection for example) each with their own classloader and which can be reclaimed after the request is complete. I can then easily imagine that they use an unload hook to record statistics about native library use, and that the statistics classes are in the top-level custom loader, and not locatable from the system loader.
While the spec makes no guarantees this will work it only says programmers "should be conservative in their use of VM services" which strongly suggests to me a "try it and see if it works" approach. In the current code while loading from the loader being reclaimed is highly dubious, delegating through that loader seems fairly reasonable to me.
That said given the spec says "unknown" the behaviour of the VM could change and still be in spec.
I presume that when using a cleaner the current classloader that would be used by FindClass is the system loader? Hence the observed behaviour of FindClass "changes" if you switch to the cleaner from the finalizer - and can't be reverted to the old behaviour by using a command-line flag. Hence if we want to be able to revert we have to do that in a FindClass-only change first. Then drop-in the cleaner update and remove the flag.
I will file a separate JBS issue to separate this JNI bug.
Okay. I see this as a RFE not a bug per-se: change from "unknown context" to a specific well known context. This case is arguable whether it's considered as a RFE or a bug because the current spec of JNI_OnUnload and JNI_FindClass are not aligned. I lean toward a bug. The bottom line: do you agree with this proposed JNI spec change?
I don't think the spec _has_ to change because I disagree that there is a misalignment between JNI_OnUnload and JNI_FindClass. FindClass clearly states it uses the current loader or else the system loader if there is
That is not accurate - sorry. FindClass doesn't actually address the possibility of being called via these load/unload hooks. See more below.
no notion of a current loader. OnUnload says it runs in an unknown context, so you don't know what the current loader may be, or even if there is one. But regardless a call to FindClass from OnUnload should use the current loader if it exists, or else the system loader. The fact it may be dubious to use the current loader when it is itself in the process of being unloaded does not impinge on the voracity of the spec in my opinion.
So you can change to using a Cleaner instead of a finalizer and while it will behave differently, that change in behaviour does not violate the spec in any way - again in my opinion.
Now if you want to pave the way for a future switch to Cleaner by changing the spec for JNI_OnUnload such that it must be executed in a context where (equivalently) there either is no current loader or the current loader is the system loader, then I do not oppose that. But the only purpose that serves is to allow a migration path to the new behaviour - and then forever locks us in.
Note however I would not want to see the implementation of FindClass having to special case this - I would hope it just happens naturally if the Cleaner thread reports the current class loader as the system loader. Does it?
I missed the fact that we already special case this for JNI_OnLoad and JNI_OnUnload. I would have thought that in the OnLoad case we would find the classloader of the class loading the native library without any need to resort to the NativeLibrary support code in ClassLoader. I guess that this: // Find calling class Klass* k = thread->security_get_caller_class(0); does not find the "caller" that I would have expected, but instead finds java.lang.System because we're executing System.loadLibrary - and hence finds the boot loader not the actual loader required. But the fact we jump through all these hoops is in itself questionable because the specification for JNI_FindClass does not indicate this will happen. It only accounts for two cases: 1. A JNI call from a declared native method - which uses the loader of the class that defines the method 2. A JNI call "through the Invocation Interface" which I interpret as being a JNI call from C code, from an attached thread, with no Java frames on the stack. In which case the system loader is used. A call from JNI_OnLoad (or OnUnload) does not, to me, fit either of these cases; nor does JNI_OnLoad say anything about the context in which it executes. So it seems we have presumed that this case should mean "use the loader of the class which loaded the native library". A very reasonable approach, but not one defined by the specification as far as I can see. But given this, it is not unreasonable to also use the same interpretation for JNI_OnUnload. So there is a gap in the specification regarding the execution context of the library lifecycle function hooks - other than onUnload being an "unknown context". David -----
Thanks, David
Mandy
On 9/27/17 5:49 AM, David Holmes wrote:
I missed the fact that we already special case this for JNI_OnLoad and JNI_OnUnload.
Yes this is buried in JNI_FindClass method.
I would have thought that in the OnLoad case we would find the classloader of the class loading the native library without any need to resort to the NativeLibrary support code in ClassLoader. I guess that this:
// Find calling class Klass* k = thread->security_get_caller_class(0);
does not find the "caller" that I would have expected, but instead finds java.lang.System because we're executing System.loadLibrary - and hence finds the boot loader not the actual loader required.
In the current implementation (without this change), NativeLibrary.load and NativeLibrary.unload native methods are the caller calling JNI_OnLoad and JNI_OnUnload respectively.
But the fact we jump through all these hoops is in itself questionable because the specification for JNI_FindClass does not indicate this will happen. It only accounts for two cases:
1. A JNI call from a declared native method - which uses the loader of the class that defines the method
2. A JNI call "through the Invocation Interface" which I interpret as being a JNI call from C code, from an attached thread, with no Java frames on the stack. In which case the system loader is used.
A call from JNI_OnLoad (or OnUnload) does not, to me, fit either of these cases; nor does JNI_OnLoad say anything about the context in which it executes. So it seems we have presumed that this case should mean "use the loader of the class which loaded the native library". A very reasonable approach, but not one defined by the specification as far as I can see. That's the whole point of this discussion and the spec needs clarification. But given this, it is not unreasonable to also use the same interpretation for JNI_OnUnload.
That might be how it ends up the current implementation.
So there is a gap in the specification regarding the execution context of the library lifecycle function hooks - other than onUnload being an "unknown context". This suggests that we should clarify in JNI_OnLoad spec to specify the context.
FYI. I file a separate issue: https://bugs.openjdk.java.net/browse/JDK-8188052 Mandy
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.01/ JNI FindClass change has been separated from this patch [1]. I made further clean up to the NativeLibrary implementation and replaced the use of Vector/Stack. I also added a native test to verify the native library can be unloaded and reloaded. Summary: The patch replaces the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded. thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049389.htm...
Hi Mandy On 5/10/2017 9:24 AM, mandy chung wrote:
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.01/
JNI FindClass change has been separated from this patch [1]. I made further clean up to the NativeLibrary implementation and replaced the use of Vector/Stack. I also added a native test to verify the native library can be unloaded and reloaded.
Summary: The patch replaces the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded.
src/java.base/share/classes/java/lang/ClassLoader.java This comment is not a sentence: 2442 /* If the library is being loaded (must be by the same thread, 2443 * because Runtime.load and Runtime.loadLibrary are 2444 * synchronous). Overall the changes seem fine, but I do have a concern about the use of ConcurrentHashMap. This would seem to be a significant change in the initialization order - and I'm wondering how libjava itself actually gets loaded ?? --- I assume the tests will be updated to use JNI_VERSION_10, assuming you push the FindClass changes first? libnativeLibraryTest.c: Not sure I see the point in the FindClass("java/lang/ClassLoader") as if it fails to find it then surely it already failed to find "java/lang/Error" and you'll possibly crash trying to throw. NativeLibraryTest.java: Not clear how/if this actually verifies any unloading actually takes place? Also if the OnUnload throws an error and that happens in the Cleaner thread, how would that exception result in the test reporting a failure ?? I think OnUnload has to update some state stored in the main test class so that it can confirm the unloading occurred successfully, or not. Thanks, David
thanks Mandy [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-October/049389.htm...
On 10/4/17 6:21 PM, David Holmes wrote:
Hi Mandy
On 5/10/2017 9:24 AM, mandy chung wrote:
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.01/
JNI FindClass change has been separated from this patch [1]. I made further clean up to the NativeLibrary implementation and replaced the use of Vector/Stack. I also added a native test to verify the native library can be unloaded and reloaded.
Summary: The patch replaces the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded.
src/java.base/share/classes/java/lang/ClassLoader.java
This comment is not a sentence:
2442 /* If the library is being loaded (must be by the same thread, 2443 * because Runtime.load and Runtime.loadLibrary are 2444 * synchronous).
This is an existing comment. I rewrite it in the updated webrev.
Overall the changes seem fine, but I do have a concern about the use of ConcurrentHashMap. This would seem to be a significant change in the initialization order -
ConcurrentHashMap is referenced in ClassLoader but actually not loaded until later. So it does change the initialization order. I now change the implementation to lazily create CHM. This saves the footprint when a class loader does not load any native library.
and I'm wondering how libjava itself actually gets loaded ??
os::native_java_library() which is called by JDK_Version_init() at VM creation time.
---
I assume the tests will be updated to use JNI_VERSION_10, assuming you push the FindClass changes first?
JDK-8188052 will be pushed first to jdk10/hs. This fix will have to wait until JDK-8188052 is integrated to jdk10/master. I will update the test to use JNI_VERSION_10 before I push.
libnativeLibraryTest.c:
Not sure I see the point in the FindClass("java/lang/ClassLoader") as if it fails to find it then surely it already failed to find "java/lang/Error" and you'll possibly crash trying to throw.
I agree that should be cleaned up. I took out FindClass("java/lang/ClassLoader") case.
NativeLibraryTest.java:
Not clear how/if this actually verifies any unloading actually takes place?
If the native library is not unloaded, p.Test.<clinit> will fail when reloading the native library. I think this is adequate. I added further checks to verify the number of times the native library is unloaded as you suggest below.
Also if the OnUnload throws an error and that happens in the Cleaner thread, how would that exception result in the test reporting a failure ??
Good point. Cleaner ignores all exceptions. I change it to be a fatal error.
I think OnUnload has to update some state stored in the main test class so that it can confirm the unloading occurred successfully, or not.
I added this per your suggestion to enhance the verification. Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.02/ Mandy
Hi Mandy, On 5/10/2017 3:14 PM, mandy chung wrote:
On 10/4/17 6:21 PM, David Holmes wrote:
Hi Mandy
On 5/10/2017 9:24 AM, mandy chung wrote:
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.01/
JNI FindClass change has been separated from this patch [1]. I made further clean up to the NativeLibrary implementation and replaced the use of Vector/Stack. I also added a native test to verify the native library can be unloaded and reloaded.
Summary: The patch replaces the ClassLoader use of finalizer with phantom reference, specifically Cleaner, for unloading native libraries. It registers the class loader for cleanup only if it's not a builtin class loader which will never be unloaded.
src/java.base/share/classes/java/lang/ClassLoader.java
This comment is not a sentence:
2442 /* If the library is being loaded (must be by the same thread, 2443 * because Runtime.load and Runtime.loadLibrary are 2444 * synchronous).
This is an existing comment. I rewrite it in the updated webrev.
Sorry I didn't notice that was refactored code. Thanks for the rewrite.
Overall the changes seem fine, but I do have a concern about the use of ConcurrentHashMap. This would seem to be a significant change in the initialization order -
ConcurrentHashMap is referenced in ClassLoader but actually not loaded
It was loaded in clinit: 2689 private static Map<String, NativeLibrary> systemNativeLibraries 2690 = new ConcurrentHashMap<>(); which was my concern.
until later. So it does change the initialization order. I now change the implementation to lazily create CHM. This saves the footprint when a class loader does not load any native library.
Okay. But now I'm wondering about whether systemNativeLibraries/nativeLibraries need to be volatile? Just looking in ClassLoader it would seem yes, but if the only paths to loading a library are through Runtime, and those methods are synchronized on the Runtime instance, then everything is serialized anyway. Are there other paths that might allow concurrent access? (If the synchronization in Runtime guarantees only a single library can ever be loaded at a time across all classloaders, then you don't even need the synchronization on the loadedLibraryNames ??).
and I'm wondering how libjava itself actually gets loaded ??
os::native_java_library() which is called by JDK_Version_init() at VM creation time.
Okay a different mechanism - and presumably no JNI_OnLoad function to execute :)
---
I assume the tests will be updated to use JNI_VERSION_10, assuming you push the FindClass changes first?
JDK-8188052 will be pushed first to jdk10/hs. This fix will have to wait until JDK-8188052 is integrated to jdk10/master. I will update the test to use JNI_VERSION_10 before I push.
Ok.
libnativeLibraryTest.c:
Not sure I see the point in the FindClass("java/lang/ClassLoader") as if it fails to find it then surely it already failed to find "java/lang/Error" and you'll possibly crash trying to throw.
I agree that should be cleaned up. I took out FindClass("java/lang/ClassLoader") case.
So is: 68 excls = (*env)->FindClass(env, "java/lang/Error"); 69 if (excls == NULL) { 70 (*env)->FatalError(env, "Error class not found"); 71 } just a sanity check that you can in fact load a class? 75 (*env)->FatalError(env, "Error class not found"); That's the wrong error message.
NativeLibraryTest.java:
Not clear how/if this actually verifies any unloading actually takes place?
If the native library is not unloaded, p.Test.<clinit> will fail when reloading the native library. I think this is adequate. I added further
Ah I hadn't realized that part.
checks to verify the number of times the native library is unloaded as you suggest below.
Also if the OnUnload throws an error and that happens in the Cleaner thread, how would that exception result in the test reporting a failure ??
Good point. Cleaner ignores all exceptions. I change it to be a fatal error.
I think OnUnload has to update some state stored in the main test class so that it can confirm the unloading occurred successfully, or not.
I added this per your suggestion to enhance the verification.
Okay. Not sure why you needed to introduce q.Bridge - I find the test logic rather hard to follow, not clear who calls what method from where and when. I was thinking of a simple public int loadedCount; public int unloadedCount; that onLoad/OnUnload would increment. Thanks, David -----
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.02/
Mandy
Hi David, This version only modifies ClassLoader.java and the new test: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.03/ On 10/4/17 11:06 PM, David Holmes wrote:
Hi Mandy,
Overall the changes seem fine, but I do have a concern about the use of ConcurrentHashMap. This would seem to be a significant change in the initialization order -
ConcurrentHashMap is referenced in ClassLoader but actually not loaded
I missed the first sentence "I misread the source previously that CHM is already loaded at clinit time. But I figure that ....". So I fixed it to lazily initialize it when the first native library is loaded.
It was loaded in clinit:
2689 private static Map<String, NativeLibrary> systemNativeLibraries 2690 = new ConcurrentHashMap<>();
which was my concern.
I got that. Thanks for bringing this up.
until later. So it does change the initialization order. I now change the implementation to lazily create CHM. This saves the footprint when a class loader does not load any native library.
Okay. But now I'm wondering about whether systemNativeLibraries/nativeLibraries need to be volatile?
It's read for JNI native method binding (ClassLoader::findNative method) and therefore I used double-locking idiom instead to avoid any performance impact. I simplified this further. The native libraries map is created with the loadedLibraryNames lock.
Just looking in ClassLoader it would seem yes, but if the only paths to loading a library are through Runtime, and those methods are synchronized on the Runtime instance, then everything is serialized anyway. Are there other paths that might allow concurrent access? (If the synchronization in Runtime guarantees only a single library can ever be loaded at a time across all classloaders, then you don't even need the synchronization on the loadedLibraryNames ??).
That's a good point. This code including the synchronization on loadedLibraryNames was written long ago. I prefer to file a JBS issue to study the locking more closely.
libnativeLibraryTest.c:
Not sure I see the point in the FindClass("java/lang/ClassLoader") as if it fails to find it then surely it already failed to find "java/lang/Error" and you'll possibly crash trying to throw.
I agree that should be cleaned up. I took out FindClass("java/lang/ClassLoader") case.
So is:
68 excls = (*env)->FindClass(env, "java/lang/Error"); 69 if (excls == NULL) { 70 (*env)->FatalError(env, "Error class not found"); 71 }
just a sanity check that you can in fact load a class? 75 (*env)->FatalError(env, "Error class not found");
That's the wrong error message.
Thanks for catching this. I revised the test and fixed the above.
Okay. Not sure why you needed to introduce q.Bridge -
I agree it's not needed any more. Initially I was trying to have a method to return a count. But later modified to a different method.
I find the test logic rather hard to follow, not clear who calls what method from where and when. I was thinking of a simple
public int loadedCount; public int unloadedCount;
that onLoad/OnUnload would increment. Well the test intends to verify if the native library can be reloaded (i.e. it has to be unloaded before reloading; otherwise UnsatifisedLinkError will be thrown). It has a native count method and it would fail if it is not loaded.
I revise the test a little. p.Test.run() loads the native library. The test checks if the native count method returns the correct value (i.e. the native library is loaded and OnLoad is invoked). Calling run() again should get ULE since it can't be reloaded if already loaded. I keep the unloadedCount which is not strictly necessary but it is an additional sanity check. I added more comment and hope it is clearer now. Mandy
Hi Mandy, On 6/10/2017 8:24 AM, mandy chung wrote:
Hi David,
This version only modifies ClassLoader.java and the new test: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.03/
This seems mostly fine now - thanks for simplifying and clarifying the test operation. I agree we can look at the overall locking strategy separately. Regarding the need for volatile to make the double-checked locking correct ... I can't quite convince myself that any thread that tries to execute a native method and so calls back to findNative from the VM, must have already gone through the code that ensured the native library maps have been initialized. I think they must, but I can't quite convince myself of it. :( Unless you can dispel my doubts I think we need to make the fields volatile to be strictly safe. That said I'm not sure lazy initialization is really worthwhile in the first place. Is the CHM creation very costly? Should we be sizing them explicitly? (More like the default Vector size used previously?) Thanks, David -----
On 10/4/17 11:06 PM, David Holmes wrote:
Hi Mandy,
Overall the changes seem fine, but I do have a concern about the use of ConcurrentHashMap. This would seem to be a significant change in the initialization order -
ConcurrentHashMap is referenced in ClassLoader but actually not loaded
I missed the first sentence "I misread the source previously that CHM is already loaded at clinit time. But I figure that ....". So I fixed it to lazily initialize it when the first native library is loaded.
It was loaded in clinit:
2689 private static Map<String, NativeLibrary> systemNativeLibraries 2690 = new ConcurrentHashMap<>();
which was my concern.
I got that. Thanks for bringing this up.
until later. So it does change the initialization order. I now change the implementation to lazily create CHM. This saves the footprint when a class loader does not load any native library.
Okay. But now I'm wondering about whether systemNativeLibraries/nativeLibraries need to be volatile?
It's read for JNI native method binding (ClassLoader::findNative method) and therefore I used double-locking idiom instead to avoid any performance impact. I simplified this further. The native libraries map is created with the loadedLibraryNames lock.
Just looking in ClassLoader it would seem yes, but if the only paths to loading a library are through Runtime, and those methods are synchronized on the Runtime instance, then everything is serialized anyway. Are there other paths that might allow concurrent access? (If the synchronization in Runtime guarantees only a single library can ever be loaded at a time across all classloaders, then you don't even need the synchronization on the loadedLibraryNames ??).
That's a good point. This code including the synchronization on loadedLibraryNames was written long ago. I prefer to file a JBS issue to study the locking more closely.
libnativeLibraryTest.c:
Not sure I see the point in the FindClass("java/lang/ClassLoader") as if it fails to find it then surely it already failed to find "java/lang/Error" and you'll possibly crash trying to throw.
I agree that should be cleaned up. I took out FindClass("java/lang/ClassLoader") case.
So is:
68 excls = (*env)->FindClass(env, "java/lang/Error"); 69 if (excls == NULL) { 70 (*env)->FatalError(env, "Error class not found"); 71 }
just a sanity check that you can in fact load a class? 75 (*env)->FatalError(env, "Error class not found");
That's the wrong error message.
Thanks for catching this. I revised the test and fixed the above.
Okay. Not sure why you needed to introduce q.Bridge -
I agree it's not needed any more. Initially I was trying to have a method to return a count. But later modified to a different method.
I find the test logic rather hard to follow, not clear who calls what method from where and when. I was thinking of a simple
public int loadedCount; public int unloadedCount;
that onLoad/OnUnload would increment. Well the test intends to verify if the native library can be reloaded (i.e. it has to be unloaded before reloading; otherwise UnsatifisedLinkError will be thrown). It has a native count method and it would fail if it is not loaded.
I revise the test a little. p.Test.run() loads the native library. The test checks if the native count method returns the correct value (i.e. the native library is loaded and OnLoad is invoked). Calling run() again should get ULE since it can't be reloaded if already loaded.
I keep the unloadedCount which is not strictly necessary but it is an additional sanity check. I added more comment and hope it is clearer now.
Mandy
Hi David, Thanks for raising the lazy initialization issue. The recent version of ClassLoader.java in webrev.03 is uploaded in place: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.03/ The native libraries map is now created lazily with synchronization. I keep the lazy initialization that will save to create a CHM as many custom class loaders don't have native code. I think it's a good saving. In addition, if we iniitialize the static systemNativeLibraries at <clinit> time, it may want to avoid using CHM as it changes the class initialization order. thanks Mandy
Hi Mandy, This all looks fine to me now. Thanks, David On 7/10/2017 5:37 AM, mandy chung wrote:
Hi David,
Thanks for raising the lazy initialization issue. The recent version of ClassLoader.java in webrev.03 is uploaded in place: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.03/
The native libraries map is now created lazily with synchronization. I keep the lazy initialization that will save to create a CHM as many custom class loaders don't have native code. I think it's a good saving. In addition, if we iniitialize the static systemNativeLibraries at <clinit> time, it may want to avoid using CHM as it changes the class initialization order.
thanks Mandy
On 06/10/2017 20:37, mandy chung wrote:
:
The native libraries map is now created lazily with synchronization. I keep the lazy initialization that will save to create a CHM as many custom class loaders don't have native code. I think it's a good saving. In addition, if we iniitialize the static systemNativeLibraries at <clinit> time, it may want to avoid using CHM as it changes the class initialization order.
Alternatively change nativeLibraries and systemNativeLibraries to volatile so the synchronization is only needed to initialize them. Otherwise this version (webrev.03) looks good to me. -Alan
On 10/09/2017 10:20 AM, Alan Bateman wrote:
On 06/10/2017 20:37, mandy chung wrote:
:
The native libraries map is now created lazily with synchronization. I keep the lazy initialization that will save to create a CHM as many custom class loaders don't have native code. I think it's a good saving. In addition, if we iniitialize the static systemNativeLibraries at <clinit> time, it may want to avoid using CHM as it changes the class initialization order.
Alternatively change nativeLibraries and systemNativeLibraries to volatile so the synchronization is only needed to initialize them. Otherwise this version (webrev.03) looks good to me.
-Alan
Yes Mandy, you could use volatile fields + double checked locking for initialization. In addition, the initializers to 'null' value are not needed / are a waste of instructions (the default is guaranteed by JLS): 2695 // Native libraries belonging to system classes. 2696 private static Map<String, NativeLibrary> systemNativeLibraries = null; 2697 2698 // Native libraries associated with the class loader. 2699 private Map<String, NativeLibrary> nativeLibraries = null; Regards, Peter
On 10/9/17 3:47 AM, Peter Levart wrote:
On 10/09/2017 10:20 AM, Alan Bateman wrote:
On 06/10/2017 20:37, mandy chung wrote:
:
The native libraries map is now created lazily with synchronization. I keep the lazy initialization that will save to create a CHM as many custom class loaders don't have native code. I think it's a good saving. In addition, if we iniitialize the static systemNativeLibraries at <clinit> time, it may want to avoid using CHM as it changes the class initialization order.
Alternatively change nativeLibraries and systemNativeLibraries to volatile so the synchronization is only needed to initialize them. Otherwise this version (webrev.03) looks good to me.
-Alan
Yes Mandy, you could use volatile fields + double checked locking for initialization.
Since this might affect JNI binding, I have avoided changing it to volatile in this patch until we get some performance number (I might be overly conversative). I prefer to follow up this together in the lock cleanup RFE that David suggests.
In addition, the initializers to 'null' value are not needed / are a waste of instructions (the default is guaranteed by JLS):
2695 // Native libraries belonging to system classes. 2696 private static Map<String, NativeLibrary> systemNativeLibraries = null; 2697 2698 // Native libraries associated with the class loader. 2699 private Map<String, NativeLibrary> nativeLibraries = null;
sure. Mandy
David, Peter, Alan The VM has a fast path to search the symbol from libjava.so first for bootstrap loader. That was the case I mostly concern about performance and it's not impacted by this change. Also I consulted with Claes on the performance impact. I took your suggestion and made systemNativeLibraries and nativeLibraries volatile. Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.04 thanks Mandy On 10/9/17 1:20 AM, Alan Bateman wrote:
Alternatively change nativeLibraries and systemNativeLibraries to volatile so the synchronization is only needed to initialize them. Otherwise this version (webrev.03) looks good to me.
-Alan
On 10/10/2017 6:17 AM, mandy chung wrote:
David, Peter, Alan
The VM has a fast path to search the symbol from libjava.so first for bootstrap loader. That was the case I mostly concern about performance and it's not impacted by this change. Also I consulted with Claes on the performance impact. I took your suggestion and made systemNativeLibraries and nativeLibraries volatile.
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.04
Looks good. Thanks, David -----
thanks Mandy
On 10/9/17 1:20 AM, Alan Bateman wrote:
Alternatively change nativeLibraries and systemNativeLibraries to volatile so the synchronization is only needed to initialize them. Otherwise this version (webrev.03) looks good to me.
-Alan
On 09/10/2017 21:17, mandy chung wrote:
David, Peter, Alan
The VM has a fast path to search the symbol from libjava.so first for bootstrap loader. That was the case I mostly concern about performance and it's not impacted by this change. Also I consulted with Claes on the performance impact. I took your suggestion and made systemNativeLibraries and nativeLibraries volatile.
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.04 This update looks good to me.
-Alan
On 10/09/2017 10:17 PM, mandy chung wrote:
David, Peter, Alan
The VM has a fast path to search the symbol from libjava.so first for bootstrap loader. That was the case I mostly concern about performance and it's not impacted by this change. Also I consulted with Claes on the performance impact. I took your suggestion and made systemNativeLibraries and nativeLibraries volatile.
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.04
Looks good now. Just one question (for a possible follow-up future optimization): 2674 private static long findNative(ClassLoader loader, String name) { 2675 Map<String, NativeLibrary> libs = 2676 loader != null ? loader.nativeLibraries() : systemNativeLibraries(); 2677 if (libs.isEmpty()) 2678 return 0; 2679 2680 // the native libraries map may be updated in another thread 2681 // when a native library is being loaded. No symbol will be 2682 // searched from it yet. 2683 for (NativeLibrary lib : libs.values()) { 2684 long entry = lib.findEntry(name); 2685 if (entry != 0) return entry; 2686 } 2687 return 0; 2688 } Now that (system)nativeLibraries is a Map, is it still necessary to iterate it and call lib.findEntry(name) on each NativeLibrary until the one that returns a non-zero entry or would it be semantically equivalent to 1st look-up the Map by the 'name' key to get the correct NativeLibrary? Regards, Peter
On 10/10/17 2:29 AM, Peter Levart wrote:
On 10/09/2017 10:17 PM, mandy chung wrote:
David, Peter, Alan
The VM has a fast path to search the symbol from libjava.so first for bootstrap loader. That was the case I mostly concern about performance and it's not impacted by this change. Also I consulted with Claes on the performance impact. I took your suggestion and made systemNativeLibraries and nativeLibraries volatile.
Updated webrev: http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.04
Looks good now. Just one question (for a possible follow-up future optimization):
2674 private static long findNative(ClassLoader loader, String name) { 2675 Map<String, NativeLibrary> libs = 2676 loader != null ? loader.nativeLibraries() : systemNativeLibraries(); 2677 if (libs.isEmpty()) 2678 return 0; 2679 2680 // the native libraries map may be updated in another thread 2681 // when a native library is being loaded. No symbol will be 2682 // searched from it yet. 2683 for (NativeLibrary lib : libs.values()) { 2684 long entry = lib.findEntry(name); 2685 if (entry != 0) return entry; 2686 } 2687 return 0; 2688 }
Now that (system)nativeLibraries is a Map, is it still necessary to iterate it and call lib.findEntry(name) on each NativeLibrary until the one that returns a non-zero entry or would it be semantically equivalent to 1st look-up the Map by the 'name' key to get the correct NativeLibrary?
The name parameter is the symbol name but not the library name. It's confusing and therefore I renamed NativeLibrary::find to NativeLibrary::findEntry. I could rename the name parameter to entryName to make it clearer. Mandy
Hi Mandy, On 10/10/17 17:00, mandy chung wrote:
Now that (system)nativeLibraries is a Map, is it still necessary to iterate it and call lib.findEntry(name) on each NativeLibrary until the one that returns a non-zero entry or would it be semantically equivalent to 1st look-up the Map by the 'name' key to get the correct NativeLibrary?
The name parameter is the symbol name but not the library name. It's confusing and therefore I renamed NativeLibrary::find to NativeLibrary::findEntry. I could rename the name parameter to entryName to make it clearer.
Mandy
Oh, I see. I haven't checked the findNative VM call-back method purpose before. It's reasonable to try all the native libraries of a ClassLoader when looking for a symbol that links to native method implementation. Constructing and keeping an index of symbols for each native library on the Java side is probably not worth since there might be lots of them in libraries and only some of them link to native method implementations and there's typically not many native libraries loaded. Regards, Peter
On 10/10/17 2:28 PM, Peter Levart wrote:
Hi Mandy,
On 10/10/17 17:00, mandy chung wrote:
Now that (system)nativeLibraries is a Map, is it still necessary to iterate it and call lib.findEntry(name) on each NativeLibrary until the one that returns a non-zero entry or would it be semantically equivalent to 1st look-up the Map by the 'name' key to get the correct NativeLibrary?
The name parameter is the symbol name but not the library name. It's confusing and therefore I renamed NativeLibrary::find to NativeLibrary::findEntry. I could rename the name parameter to entryName to make it clearer.
Mandy
Oh, I see. I haven't checked the findNative VM call-back method purpose before. It's reasonable to try all the native libraries of a ClassLoader when looking for a symbol that links to native method implementation. Constructing and keeping an index of symbols for each native library on the Java side is probably not worth since there might be lots of them in libraries and only some of them link to native method implementations and there's typically not many native libraries loaded. Exactly.
Mandy
participants (6)
-
Alan Bateman
-
Brent Christian
-
David Holmes
-
Kim Barrett
-
mandy chung
-
Peter Levart