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
