On 29/10/2014 4:04 PM, Ioi Lam wrote:
On 10/28/14, 7:34 PM, David Holmes wrote:
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.
I copied the code from ClassLoader.loadClass, which does it it a
synchronized block:
>
class ClassLoader {
protected Class<?> loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
synchronized (getClassLoadingLock(name)) {
// First, check if the class has already been loaded
Class<?> c = findLoadedClass(name);
if (c == null) {
long t0 = System.nanoTime();
try {
if (parent != null) {
c = parent.loadClass(name, false);
} else {
c = findBootstrapClassOrNull(name);
}
} catch (ClassNotFoundException e) {
// ClassNotFoundException thrown if class not found
// from the non-null parent class loader
}
if (c == null) {
// If still not found, then invoke findClass in order
// to find the class.
long t1 = System.nanoTime();
c = findClass(name);
// this is the defining class loader; record the stats
sun.misc.PerfCounter.getParentDelegationTime().addTime(t1 - t0);
sun.misc.PerfCounter.getFindClassTime().addElapsedTimeFrom(t1);
sun.misc.PerfCounter.getFindClasses().increment();
}
}
if (resolve) {
resolveClass(c);
}
return c;
}
}
So I guess it should look like this in Launcher$AppClassLoader, just to
ensure the same things are done (regardless of whether it's necessary or
not)?
In ClassLoader.loadClass it is providing atomicity across a number of
actions in the worst-case:
- checking for already loaded; if not then
- try to load through parent; if not then
- findClass (which will do defineClass)
You don't have those atomicity constraints because you are only doing
one thing - checking to see if the class is loaded.
Your locking is probably harmless but those are famous last words when
it comes to classloading. :)
Does resolveClass need to be done inside the synchronized block?
Depends on whether it depends on the classloader locking to prevent
concurrent resolve attempts.
David
-----
class Launcher$AppClassLoader {
public Class<?> loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
int i = name.lastIndexOf('.');
if (i != -1) {
SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPackageAccess(name.substring(0, i));
}
}
if (ucp.knownToNotExist(name)) {
// The class of the given name is not found in the parent
// class loader as well as its local URLClassPath.
// Check if this class has already been defined
dynamically;
// if so, return the loaded class; otherwise, skip the
parent
// delegation and findClass.
>>from here
* synchronized (getClassLoadingLock(name)) {**
** Class<?> c = findLoadedClass(name);**
** if (c != null) {**
** if (resolve) {**
** resolveClass(c);**
** }**
** return c;**
** }**
** }*
<<to here
throw new ClassNotFoundException(name);
}
return (super.loadClass(name, resolve));
}
Thanks
- Ioi
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
