[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round
From: Karen Kinnear <karen.kinnear () oracle ! com>
Date: 2014-10-30 13:32:12
Message-ID: 983C3BC3-6D2D-4C41-9F35-B11B6C6DEC84 () oracle ! com
[Download RAW message or body]
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
> > > > > > > >
> > > > > > > >
> > > > > > > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic