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

[David Holmes]

Hi Mandy,

On 6/10/2017 8:24 AM, mandy chung wrote:
> Hi David,
> 
> This version only modifies ClassLoader.java and the new test:
> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8164512/webrev.03/

This seems mostly fine now - thanks for simplifying and clarifying the 
test operation.

I agree we can look at the overall locking strategy separately.

Regarding the need for volatile to make the double-checked locking 
correct ... I can't quite convince myself that any thread that tries to 
execute a native method and so calls back to findNative from the VM, 
must have already gone through the code that ensured the native library 
maps have been initialized. I think they must, but I can't quite 
convince myself of it. :( Unless you can dispel my doubts I think we 
need to make the fields volatile to be strictly safe.

That said I'm not sure lazy initialization is really worthwhile in the 
first place. Is the CHM creation very costly? Should we be sizing them 
explicitly? (More like the default Vector size used previously?)

Thanks,
David
-----

> 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