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