RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Hi David, I have update the latest webrev at:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/
and fixed the "int cache[]" style you mentioned.
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
Thanks
- Ioi
On 10/26/14, 4:27 PM, David Holmes wrote:
Style nit: all the
int cache[]
should be
int[] cache
Also not clear if 8061651-lookup-index-open-v2 is latest webrev ??
Thanks,
David
On 25/10/2014 9:38 AM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. please see comments in-line
On 10/24/14, 2:33 PM, Mandy Chung wrote:
On 10/23/2014 9:34 PM, Ioi Lam wrote:
Hi Mandy,
Thanks for the review. I have updated the patch:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/
Thanks for removing the LoaderType enum. Two questions -
do you need the new hasLookupCacheLoader variable? Should
lookupCAcheURLs be always be non-null if it has the lookup
cache?
I removed hasLookupCacheLoader and changed the condition check to
(lookupCacheURLs != null)
Most methods referencinglookupCacheURLs and lookupCacheLoader
are synchronized except initLookupCache and knowToNotExist.
Should they? Or volatile?
I changed both to synchronized.
On 10/21/14, 12:56 PM, Mandy Chung wrote:
line 398: what happens if loaders.size() > maxindex? Shouldn't
it return null?
In this case, all the loaders that's needed by the cache[] have been
opened. So we should return cache[].
I forget about that, sorry.I'm thinking how to make it more
obvious to those who doesn't know much of the details. Every time I
read this and I need to reload what I know about and miss something.
I changed the code to this. What do you think?
It seems to me that it would help if you refactor and add a method
"ensureLoadersInited" that will make it clear what it attempts to do.
The side effect invalidating the cache can be checked after it's
called and no need to worry about lookupCacheEnabled must be checked
in any order. Something like this
if (cache != null && cache.length > 0) {
int maxindex = cache[cache.length - 1];
ensureLoaderOpened(maxindex);
if (loaders.get(maxindex) == null || !lookupCacheEnabled) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders FAILED " +
loaders.size() + " for maxindex=" +
maxindex);
}
return null;
}
...
}
return cache;
I changed the code to
private synchronized int[] getLookupCache(String name) {
if (lookupCacheURLs == null || !lookupCacheEnabled) {
return null;
}
int cache[] = getLookupCacheForClassLoader(lookupCacheLoader,
name);
if (cache != null && cache.length > 0) {
int maxindex = cache[cache.length - 1]; // cache[] is
strictly ascending.
if (!ensureLoaderOpened(maxindex)) {
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders FAILED " +
loaders.size() + " for
maxindex=" + maxindex);
}
return null;
}
}
return cache;
}
private boolean ensureLoaderOpened(int index) {
if (loaders.size() <= index) {
// Open all Loaders up to, and including, index
if (getLoader(index) == null) {
return false;
}
if (!lookupCacheEnabled) {
// cache was invalidated as the result of the above
call.
return false;
}
if (DEBUG_LOOKUP_CACHE) {
System.out.println("Expanded loaders " +
loaders.size() +
" to index=" + index);
}
}
return true;
}
The code is getting further complicated with this lookup cache and
bear with me for being picky.
if (loaders.size() <= maxindex) {
boolean failed = false;
// Call getNextLoader() with a null cache to open all
// Loaders up to, and including, maxindex.
if (getNextLoader(null, maxindex) == null) {
Can this call getLoader(maxindex)? They are essentially the same.
I think getLoader(maxindex) makes it explicit that it forces to
open the loader.
Changed.
Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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. 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. Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Hi Ioi,
I have a question for following code in AppClassLoader.loadClass(). If a
class is 'known to not exist' for the AppClassLoader (not in the
AppClassLoader and parent classloaders' shared lookup cache), it seems
findLoadedClass() would only find classes that's dynamically loaded by
the parent loaders. The AppClassLoader would never try to load the
class. Is the AppClassLoader's lookup cache guaranteed to have all the
classes in it's path?
315 if (ucp.knownToNotExist(name)) {
316 // The class of the given name is not found in the parent
317 // class loader as well as its local URLClassPath.
318 // Check if this class has already been defined
dynamically;
319 // if so, return the loaded class; otherwise, skip the
parent
320 // delegation and findClass.
321 synchronized (getClassLoadingLock(name)) {
322 Class c = findLoadedClass(name);
323 if (c != null) {
324 return c;
325 }
326 }
327 throw new ClassNotFoundException(name);
328 }
Thanks,
Jiangli
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/
and fixed the "int cache[]" style you mentioned.
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
Thanks
- Ioi
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
On 10/27/2014 9:38 PM, Ioi Lam wrote:
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.
No, set it in the command-line (i.e. @run) is fine.
Removing line 44-58 should do it.
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.
It will call getLookupCacheURLs. It's just a sanity test and it's fine
to call one but not all three.
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?
As long as A.test and B.test are invoked and finishes, it should be
adequate.
Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
On 10/27/14, 7:52 PM, Jiangli Zhou wrote:
Hi Ioi,
I have a question for following code in AppClassLoader.loadClass(). If
a class is 'known to not exist' for the AppClassLoader (not in the
AppClassLoader and parent classloaders' shared lookup cache), it seems
findLoadedClass() would only find classes that's dynamically loaded by
the parent loaders. The AppClassLoader would never try to load the
class. Is the AppClassLoader's lookup cache guaranteed to have all the
classes in it's path?
315 if (ucp.knownToNotExist(name)) {
316 // The class of the given name is not found in
the parent
317 // class loader as well as its local URLClassPath.
318 // Check if this class has already been defined
dynamically;
319 // if so, return the loaded class; otherwise,
skip the parent
320 // delegation and findClass.
321 synchronized (getClassLoadingLock(name)) {
322 Class c = findLoadedClass(name);
323 if (c != null) {
324 return c;
325 }
326 }
327 throw new ClassNotFoundException(name);
328 }
The reason is to make the behavior consistent with
java.lang.ClassLoader.loadClass():
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 is not in any of the JAR files but was dynamically defined
(using ClassLoader.defineClass, etc), then AppClassLoader.loadClass()
should return the class without throwing ClassNotFoundException.
Thanks
- Ioi
Thanks,
Jiangli
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/
and fixed the "int cache[]" style you mentioned.
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
Thanks
- Ioi
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
On 10/27/14, 9:52 PM, Mandy Chung wrote:
On 10/27/2014 9:38 PM, Ioi Lam wrote:
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.
No, set it in the command-line (i.e. @run) is fine.
Removing line 44-58 should do it.
I will remove the check from line 44 - 48.
I want to keep the -Dfoo.foo.bar=xyz and the corresponding check to make
sure that the JTREG framework is working as intended. Otherwise JTREG
could have skipped the -Dsun.cds.enableSharedLookupCache=true and the
test will still report "PASS" even though the intended action was not
performed.
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.
It will call getLookupCacheURLs. It's just a sanity test and it's
fine to call one but not all three.
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?
As long as A.test and B.test are invoked and finishes, it should be
adequate.
Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
On 10/27/2014 10:03 PM, Ioi Lam wrote: I will remove the check from line 44 - 48. I want to keep the -Dfoo.foo.bar=xyz and the corresponding check to make sure that the JTREG framework is working as intended. Otherwise JTREG could have skipped the -Dsun.cds.enableSharedLookupCache=true and the test will still report "PASS" even though the intended action was not performed. Now I see what the test is trying to verify: sun.cds.enableSharedLookupCache should be removed from the system properties as it's only an interface with the library. There is no harm in having the test as is while I would say -Dfoo.foo.bar=xyz is not really needed; otherwise other tests may fail if jtreg does something wrong. Approved what you have in v3. thanks Mandy
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Hi Ioi,
This sounds ok.
Thanks,
Jiangli
On 10/27/2014 09:57 PM, Ioi Lam wrote:
On 10/27/14, 7:52 PM, Jiangli Zhou wrote:
Hi Ioi,
I have a question for following code in AppClassLoader.loadClass().
If a class is 'known to not exist' for the AppClassLoader (not in the
AppClassLoader and parent classloaders' shared lookup cache), it
seems findLoadedClass() would only find classes that's dynamically
loaded by the parent loaders. The AppClassLoader would never try to
load the class. Is the AppClassLoader's lookup cache guaranteed to
have all the classes in it's path?
315 if (ucp.knownToNotExist(name)) {
316 // The class of the given name is not found in
the parent
317 // class loader as well as its local URLClassPath.
318 // Check if this class has already been defined
dynamically;
319 // if so, return the loaded class; otherwise,
skip the parent
320 // delegation and findClass.
321 synchronized (getClassLoadingLock(name)) {
322 Class c = findLoadedClass(name);
323 if (c != null) {
324 return c;
325 }
326 }
327 throw new ClassNotFoundException(name);
328 }
The reason is to make the behavior consistent with
java.lang.ClassLoader.loadClass():
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 is not in any of the JAR files but was dynamically defined
(using ClassLoader.defineClass, etc), then AppClassLoader.loadClass()
should return the class without throwing ClassNotFoundException.
Thanks
- Ioi
Thanks,
Jiangli
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/
and fixed the "int cache[]" style you mentioned.
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
Thanks
- Ioi
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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.
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
>
>
>
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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)?
Does resolveClass need to be done inside the synchronized block?
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;**
**}**
**}*
<
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.enableSharedLookupC
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
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;**
**}**
**}*
<
Davi
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
David,
On Oct 29, 2014, at 2:04 AM, 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.
findLoadedClass is always a "snapshot", i.e. I agree with you that with or
without a lock it is possible that another thread will define the class you
are looking for after the findLoadedClass returns failure.
Defined dynamically here is in contrast to being found in the archive - and
uses the normal defineClass code paths.
Going back to double-check in our parallel class loading changes to class
loader synchronization:
http://openjdk.java.net/groups/core-libs/ClassLoaderProposal.html
This does match my memory - which is that the synchronization is to prevent
duplicate defineClass calls, and we deliberately
made the following changes to java.lang.ClassLoader:
protected loadClass(String, boolean) changes:
Remove synchronized keyword
If "this" is not a parallel capable class loader, synchronize on "this" for
backward compatibility
else synchronize on a class-name-based-lock
The synchronization in protected loadClass(String, boolean) ensures that
defineClass(...) will not be called multiple times in parallel for the same
class name/class loader pair.
and recommendations for other class loaders:
4.b. Class loaders that invoke registerAsParallelCapable(), that override
protected loadClass(String, boolean) or public loadClass(String)
We recommend that you override findClass(String) instead, if at all possible
To ensure that the protected defineClass(...) method is only called once for
the same class name, you need to implement a finer-grained locking scheme. One
option would be to adopt the class name based locking mechanism from
java.lang.ClassLoader's protected loadClass(String, boolean) method.
Note also that resolveClass does nothing (it throws an exception if called with
a null value).
So - my conclusion is we do not need the lock.
David - I believe you might be improving the parallel class loading logic going
forward, exploring the possibility of
allowing parallel defineClass (with spec modifications). I don't think any
synchronization is going to help us with
that cleanup or with performance. And I think it will confuse people into
worrying that the synchronization is needed here.
If you want to leave it in anyway, please add some sort of comment that the
synchronized is paranoia rather than
having a known reason to use it - or that Karen doesn't believe it is needed
:-) - or something - to not prevent future
improvements here.
thanks,
Karen
>>
> 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) {
>
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Sorry, I was confused about who wrote what.
Sounds like David and I are in agreement that you can remove the
synchronization - I believe that
would be much cleaner.
And resolveClass does nothing and is final so no worries there.
thanks,
Karen
On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
> 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 load
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
On 10/29/2014 7:16 AM, Karen Kinnear wrote:
Sorry, I was confused about who wrote what.
Sounds like David and I are in agreement that you can remove the
synchronization - I believe that would be much cleaner.
I agree that the class loader lock is not really needed in
the knownToNotExist case as it's checking if the class is
loaded or not. Good catch, Karen.
Mandy
And resolveClass does nothing and is final so no worries there.
thanks,
Karen
On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
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;
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
OK, here's the latest version. I removed the synchronization but kept
the resolveClass just for completeness, even if it currently does nothing.
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.
Class c = findLoadedClass(name);
if (c != null) {
if (resolve) {
resolveClass(c);
}
return c;
}
throw new ClassNotFoundException(name);
}
return (super.loadClass(name, resolve));
}
Thanks
- Ioi
On 10/29/14, 12:10 PM, Mandy Chung wrote:
On 10/29/2014 7:16 AM, Karen Kinnear wrote:
Sorry, I was confused about who wrote what.
Sounds like David and I are in agreement that you can remove the
synchronization - I believe that would be much cleaner.
I agree that the class loader lock is not really needed in
the knownToNotExist case as it's checking if the class is
loaded or not. Good catch, Karen.
Mandy
And resolveClass does nothing and is final so no worries there.
thanks,
Karen
On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
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
Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Thanks Ioi - looks good.
thanks,
Karen
On Oct 30, 2014, at 1:26 AM, Ioi Lam wrote:
> OK, here's the latest version. I removed the synchronization but kept the
> resolveClass just for completeness, even if it currently does nothing.
>
>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.
>Class c = findLoadedClass(name);
>if (c != null) {
>if (resolve) {
>resolveClass(c);
>}
>return c;
>}
>throw new ClassNotFoundException(name);
>}
>
>return (super.loadClass(name, resolve));
>}
>
> Thanks
>
> - Ioi
>
> On 10/29/14, 12:10 PM, Mandy Chung wrote:
>>
>> On 10/29/2014 7:16 AM, Karen Kinnear wrote:
>>> Sorry, I was confused about who wrote what.
>>>
>>> Sounds like David and I are in agreement that you can remove the
>>> synchronization - I believe that would be much cleaner.
>>
>> I agree that the class loader lock is not really needed in
>> the knownToNotExist case as it's checking if the class is
>> loaded or not. Good catch, Karen.
>>
>> Mandy
>>
>>> And resolveClass does nothing and is final so no worries there.
>>>
>>> thanks,
>>> Karen
>>>
>>> On Oct 29, 2014, at 2:37 AM, David Holmes wrote:
>>>
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.
>
