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
