Review Request JDK-8164512: Replace ClassLoader use of finalizer with phantom reference to unload native library

[David Holmes]

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
>