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

[coleen.phillimore at oracle.com]

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.

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