RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

[David Holmes]

Looks good!

Thanks,
David

On 13/02/2018 6:50 AM, harold seigel wrote:
> Hi Karen,
> 
> Thanks for looking at this!
> 
> I re-ran the ParallelClassLoading tests with no options, with 
> -XX:+AllowParallelDefineClass, and with -XX:+AlwaysLockClassLoader, and 
> all the tests passed.  I also determined that the few Mach5 regression 
> test failures that I encountered were unrelated to my change.
> 
> Please see this latest webrev that adds 12 as an expiration date, 
> removes the 'guarantee' section, and fixes the comment at line 786 of 
> systemDictionary.cpp.
> 
>     http://cr.openjdk.java.net/~hseigel/bug_8184289.3/webrev/index.html
> 
> Thanks, Harold
> 
> On 2/12/2018 2:39 PM, Karen Kinnear wrote:
>> Harold,
>>
>> Thanks for doing this.
>>
>> I think you told me that
>> 1) the version change has made it in
>> 2) you also put 12 as an expiration date
>> 3) you are running the ParallelClassLoading tests with the remaining 
>> two flags (you’ve already
>> run them without any flags):
>>   AllowParallelDefineClass = true and AlwaysLockClassLoader=true
>>
>> In terms of the guarantee in question
>> // For UnsyncloadClass only
>>   848       // If they got a linkageError, check if a parallel class load succeeded.
>>   849       // If it did, then for bytecode resolution the specification requires
>>   850       // that we return the same result we did for the other thread, i.e. the
>>   851       // successfully loaded InstanceKlass
>>   852       // Should not get here for classloaders that support parallelism
>>   853       // with the new cleaner mechanism, even with AllowParallelDefineClass
>>   854       // Bootstrap goes through here to allow for an extra guarantee check
>> 855 if (UnsyncloadClass || (class_loader.is_null())) {
>>   856         if (k == NULL && HAS_PENDING_EXCEPTION
>>   857           && PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
>>   858           MutexLocker mu(SystemDictionary_lock, THREAD);
>>   859           InstanceKlass* check = find_class(d_hash, name, dictionary);
>>   860           if (check != NULL) {
>>   861             // Klass is already loaded, so just use it
>>   862             k = check;
>>   863             CLEAR_PENDING_EXCEPTION;
>>   864             guarantee((!class_loader.is_null()), "dup definition for bootstrap loader?");
>>   865           }
>>   866         }
>>   867       }
>>
>>   1) I agree you can remove the entire section
>>       - the guarantee was there for future proofing in case we ever 
>> allowed parallel class loading of the
>>         same class for the null loader and to make sure I didn’t have 
>> any logic holes.
>>       - I would not put an assertion for the first half of the 
>> condition - I would remove completely
>>       - the code currently prevents parallel class loading of the same 
>> class for the null loader at:
>> resolve_instance_class_or_null see line 785 …
>>   while (!class_has_been_loaded && old probe && old 
>> probe->instance_load_in_progress()) {
>>      //  case x: bootstrap class loader: prevent futile class loading,
>>      // wait on first requestor
>>      if (class_loader.is_null()) {
>>         SystemDictionary_lock->wait();
>>
>> This logic means that there is a registered INSTANCE_LOAD on this 
>> placeholder entry.
>>
>>
>> Other minor comments (sorry if you already got these and I missed them 
>> in earlier emails)
>> - all in SystemDictionary.cpp
>>
>> 1. line 72 comment “Five cases:” -> “Four cases:”
>> So you removed case 3 and renumbered, so old references to case 4 -> 
>> case 3 ,and old references
>> to case 5 become case 4:
>>
>> So - line 786, “Case 4” -> “case 3”
>>
>> thanks,
>> Karen
>>
>>
>>> On Feb 12, 2018, at 11:13 AM, harold seigel <harold.seigel at oracle.com 
>>> <mailto:harold.seigel at oracle.com>> wrote:
>>>
>>> Hi Alan,
>>>
>>> Thanks for looking at this.
>>>
>>> Harold
>>>
>>> On 2/12/2018 2:52 AM, Alan Bateman wrote:
>>>> On 12/02/2018 06:54, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> Adding core-libs-dev so they are aware of the ClassLoader change.
>>>> Thanks, that part is okay and good to see this going away.
>>>>
>>>> -Alan
>>>
>>
>