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

[mandy chung]

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