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

[Ioi Lam]

On 9/18/19 6:47 PM, coleen.phillimore at oracle.com wrote:
>
> I have to say that the 01 assert matched better what I thought it 
> should assert, ie. that boot class loaders cannot get to that code in 
> parallel.
>
> The mirror has a bunch of other logic with sharing that just 
> complicates this basic premise.  Also, by asserting in the code that 
> takes out the lock, it doesn't make it clear that two threads with the 
> bootloader cannot execute that code in parallel.
>

But the problem with version 01 is two threads could both come to the 
assert at the same time, and both see that the class loader is not set, 
and then both would proceed to restore the class.

Anyway, maybe we should not put the assert in load_shared_class. 
Instead, move the assert to Klass::restore_unsharable_info():

http://cr.openjdk.java.net/~iklam/jdk14/8231125-shared-boot-class-parallel-test.v03/

I also fixed the spacing in the "for" loops in the other test file as 
requested by David.

Thanks
- Ioi

> I like 01 better (except the test fixes are good).
> thanks,
> Coleen
>
> On 9/18/19 2:23 AM, 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.
>>>>>
>>>>
>>
>