RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)
Mandy Chung
mandy.chung at oracle.com
Wed Oct 29 19:10:18 UTC 2014
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