Review Request JDK-8164512: Replace ClassLoader use of finalizer with phantom reference to unload native library
mandy.chung at oracle.com
Thu Oct 5 05:14:13 UTC 2017
On 10/4/17 6:21 PM, David Holmes wrote:
> Hi Mandy
> On 5/10/2017 9:24 AM, mandy chung wrote:
>> Updated webrev:
>> JNI FindClass change has been separated from this patch . 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.
> 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
> 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.
> 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
> Not clear how/if this actually verifies any unloading actually takes
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
> 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.
More information about the core-libs-dev