RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Ioi Lam
ioi.lam at oracle.com
Wed Oct 29 06:04:47 UTC 2014
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;**
** }**
** }*
<<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
>>>
>>>
>>>
>>
More information about the core-libs-dev
mailing list