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

[David Holmes]

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