Hi Karen,
I haven't been tracking the details of this and am unclear on the
overall caching strategy however ...
On 29/10/2014 8:49 AM, Karen Kinnear wrote:
Ioi,
Looks good! Thanks to all who have contributed!
A couple of minor comments/questions:
1. jvm.h (hotspot and jdk)
All three APIs talk about loader_type, but the code uses loader.
2. Launcher.java
To the best of my understanding - the call to findLoadedClass does not require
synchronizing on the class loader lock,
that is needed to ensure find/define atomicity - so that we do not call
defineClass twice on the same class - i.e. in
loadClass - it is needed around the findLoadedClass / findClass(defineClass)
calls. This call is just a SystemDictionary lookup
and does not require the lock to be held.
If the class can be defined dynamically - which it appears it can
(though I'm not sure what that means) - then you can have a race between
the thread doing the defining and the thread doing the findLoadedClass.
By doing findLoadedClass with the lock held you enforce some
serialization of the actions, but there is still a race. So the only way
the lock could matter is if user code could trigger the second thread's
lookup of the class after the lock has been taken by the thread doing
the dynamic definition - whether that is possible depends on what this
dynamic definition actually is.
David
David H and Mandy - does that make sense to you both?
thanks,
Karen
On Oct 28, 2014, at 12:38 AM, Ioi Lam wrote:
On 10/27/14, 7:04 PM, Mandy Chung wrote:
On 10/27/2014 3:32 PM, Ioi Lam wrote:
Hi David, I have update the latest webrev at:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/
The update looks good. Thanks.
This version also contains the JDK test case that Mandy requested:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html
What I request to add is a test setting the system property
(-Dsun.cds.enableSharedLookupCache=true) and continue to load class A and B.
Removing line 44-58 should do it and also no need to set -Dfoo.foo.bar.
Do you mean change the test to call System.setProperty("sun.cds.enableSharedLookupCache",
"true")?
But we know that the property is checked only once, before any app classes are
loaded. So calling System.setProperty in an application class won't test
anything.
It'd be good if you run this test and turn on the debug traces to make sure
that the application class loader and ext class loader will start up with the
lookup cache enabled and make up call to the VM. As it doesn't have the app
cds archive, it will invalidate the cache right away and continue the class
lookup with null cache array.
In the latest code, if CDS is not available, lookupCacheEnabled will be set to
false inside the static initializer of URLClassPath:
private static volatile boolean lookupCacheEnabled
= "true".equals(VM.getSavedProperty("sun.cds.enableSharedLookupCache"));
later, when the boot/ext/app loaders call into here:
synchronized void initLookupCache(ClassLoader loader) {
if ((lookupCacheURLs = getLookupCacheURLs(loader)) != null) {
lookupCacheLoader = loader;
} else {
// This JVM instance does not support lookup cache.
disableAllLookupCaches();
}
}
their lookupCacheURLs[] fields will all be set to null. As a result,
getLookupCacheForClassLoader and knownToNotExist0 will never be called.
I can add a DEBUG_LOOKUP_CACHE trace inside disableAllLookupCaches to print "lookup
cache disabled", and check for that in the test. Is this OK?
Thanks
- Ioi
