RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Ioi Lam]

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
>>>
>>>
>>>
>>