RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Ioi Lam
ioi.lam at oracle.com
Thu Oct 30 05:26:01 UTC 2014
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 hotspot-runtime-dev
mailing list