RFR(S) 8231125 Improve testing of parallel loading of shared classes by the boot class loader

Wed Sep 18 19:11:21 UTC 2019

Hi Calvin,

Thanks for the review. You're right. I changed the code to

   log("thread[%d] t = %s, c = %s, l = %s", thread_id, this, clazz, 
clazz.getClassLoader());

Now the output looks like this, which is what I want:

ParallelLoadThread: thread[3] t = Thread[Thread-3,5,main], c = class 
ParallelClass0, l = null
ParallelLoadThread: thread[2] t = Thread[Thread-2,5,main], c = class 
ParallelClass0, l = null
ParallelLoadThread: thread[1] t = Thread[Thread-1,5,main], c = class 
ParallelClass0, l = null
ParallelLoadThread: thread[0] t = Thread[Thread-0,5,main], c = class 
ParallelClass0, l = null
ParallelLoadThread: thread[0] t = Thread[Thread-0,5,main], c = class 
ParallelClass1, l = null
ParallelLoadThread: thread[3] t = Thread[Thread-3,5,main], c = class 
ParallelClass1, l = null
ParallelLoadThread: thread[1] t = Thread[Thread-1,5,main], c = class 
ParallelClass1, l = null
ParallelLoadThread: thread[2] t = Thread[Thread-2,5,main], c = class 
ParallelClass1, l = null
ParallelLoadThread: thread[0] t = Thread[Thread-0,5,main], c = class 
ParallelClass2, l = null
ParallelLoadThread: thread[1] t = Thread[Thread-1,5,main], c = class 
ParallelClass2, l = null
ParallelLoadThread: thread[3] t = Thread[Thread-3,5,main], c = class 
ParallelClass2, l = null
ParallelLoadThread: thread[2] t = Thread[Thread-2,5,main], c = class 
ParallelClass2, l = null

Thanks
- Ioi

On 9/18/19 11:42 AM, Calvin Cheung wrote:
> Hi Ioi,
>
> Just one comment on the v02 webrev:
>
> http://cr.openjdk.java.net/~iklam/jdk14/8231125-shared-boot-class-parallel-test.v02/test/hotspot/jtreg/runtime/cds/appcds/test-classes/ParallelLoad.java.sdiff.html 
>
>
> 191             log("thread[%d] t = %s, c = %s, l = %s", i, this, 
> clazz, clazz.getClassLoader());
>
> I thought i is the index into the ParallelLoad.MAX_CLASSES.
>
> Should the above i be thread_id?
>
> thanks,
>
> Calvin
>
> On 9/17/19 11:23 PM, Ioi Lam wrote:
>>
>>
>> On 9/17/19 6:27 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 18/09/2019 7:24 am, coleen.phillimore at oracle.com wrote:
>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231125-shared-boot-class-parallel-test.v01/src/hotspot/share/classfile/systemDictionary.cpp.udiff.html 
>>>>
>>>>
>>>> Can you add a line something like.
>>>>
>>>> + if (class_loader() == NULL) {
>>>> + // See case 3 in SystemDictionary::resolve_instance_class_or_null
>>>> +      // multiple threads wait on the SystemDictionary_lock if the 
>>>> boot loader class is in the placeholder table, so only
>>>> +      // one thread loads the class.
>>>> + assert(ik->class_loader_data() == NULL, "boot loader classes are 
>>>> not loaded in parallel");
>>>> + }
>>>> +
>>>
>>> Even with Coleen's suggested comment I still don't see what this 
>>> code has to do with loading in parallel? AFAICS all you are 
>>> asserting is that if the class is to be loaded by the boot-loader 
>>> then the class should have been dumped by the boot-loader - no?
>>>
>> Hi David,
>>
>> When we were analyzing a recent random crash that might be caused by 
>> race condition during class loading, we looked at this call to 
>> restore_unshareable_info() in SystemDictionary::load_shared_class()
>>
>>     ClassLoaderData* loader_data = 
>> ClassLoaderData::class_loader_data(class_loader());
>>     {
>>       HandleMark hm(THREAD);
>>       Handle lockObject = compute_loader_lock_object(class_loader, 
>> THREAD);
>>       check_loader_lock_contention(lockObject, THREAD);
>>       ObjectLocker ol(lockObject, THREAD, true);
>>       // prohibited package check assumes all classes loaded from 
>> archive call
>>       // restore_unshareable_info which calls ik->set_package()
>>       ik->restore_unshareable_info(loader_data, protection_domain, 
>> CHECK_NULL);
>>     }
>>
>> Because all the crash seemed to be with shared classes loaded by the 
>> boot loader, we suspect that two threads competing to load the same 
>> class may arrive here simultaneously, and we will end up calling 
>> ik->restore_unshareable_info() twice, with harmful side effects.
>>
>> Upon further investigation, we found that this is not the case. But 
>> it will be good to have an assert to check for this -- that we won't 
>> call ik->restore_unshareable_info() more than once.
>>
>> I realized that I put the assert at the wrong place, and asserted on 
>> the wrong field (non-boot classes will enter load_shared_class() with 
>> ik->class_loader_data() != NULL). So I changed the assert to be on 
>> the java_mirror. That way, the assert works for shared classes for 
>> all loaders.
>>
>>       assert(!ik->is_java_mirror_restored(), "don't load the same 
>> shared class twice");
>>       ik->restore_unshareable_info(loader_data, protection_domain, 
>> CHECK_NULL);
>>       assert(ik->is_java_mirror_restored() || 
>> !SystemDictionary::Class_klass_loaded(), "must be");
>>
>> Here's an updated webrev, which also contains fixes you suggested below.
>>
>> Thanks
>> - Ioi
>>
>>> The test seems okay - the proof is in the running :)
>>>
>>> One style nit:
>>>
>>> !         for (int i=0; i<APP_LOOPS; i++) {
>>>
>>> spaces around binary operators please. This seems to impact all for 
>>> loops.
>>>
>>> Also:
>>>
>>> 175             {   // Avoid logging in this block so the threads 
>>> can proceeds without
>>>
>>> s/proceeds/proceed/
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> I wonder if the ObjectLocker is really needed for non-bootclass 
>>>> loaders below this code, since it appears to not be needed for boot 
>>>> class loaders.
>>>>
>>>> The test changes look good.  I think you're supposed to have @bug 
>>>> tags in them.
>>>>
>>>> Thanks for going through this code looking for races!
>>>> Coleen
>>>>
>>>>
>>>> On 9/17/19 1:10 PM, Ioi Lam wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8231125
>>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231125-shared-boot-class-parallel-test.v01/ 
>>>>>
>>>>>
>>>>>
>>>>> [1] Modified ParallelLoadTest.java to also test the loading of 
>>>>> shared classes
>>>>>     into the boot loader.
>>>>> [2] Modified ParallelLoad.java to minimize the initial time lag 
>>>>> when the 4 parallel
>>>>>     threads compete to load the same class.
>>>>> [3] Added an assert in the JVM to verify that for a shared classes 
>>>>> loaded
>>>>>     the boot class loader, SystemDictionary::load_shared_class is 
>>>>> not called
>>>>>     multiple times.
>>>>>
>>>>
>>