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) { > // 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 >>>> >>>> >>>> >>> >
