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

Karen Kinnear karen.kinnear at oracle.com
Thu Oct 30 13:32:12 UTC 2014


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




More information about the core-libs-dev mailing list