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