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

[Karen Kinnear]

Harold,

Looks good. Many thanks!

Karen

> On Feb 12, 2018, at 3:50 PM, harold seigel <harold.seigel at oracle.com> 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 <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
>>> 
>> 
>