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

[David Holmes]

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

On 10/02/2018 5:44 AM, harold seigel wrote:
> Hi David,
> 
> Thanks for reviewing this.
> 
> Please see updated webrev: 
> http://cr.openjdk.java.net/~hseigel/bug_8184289.2/webrev/index.html

This all seems fine to me.

I agree there is question mark over the SystemDictionary code now only 
used for the null/boot loader:

  848       if ((class_loader.is_null())) {
  849         if (k == NULL && HAS_PENDING_EXCEPTION
  850           && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
  851           MutexLocker mu(SystemDictionary_lock, THREAD);
  852           InstanceKlass* check = find_class(d_hash, name, dictionary);
  853           if (check != NULL) {
  854             // Klass is already loaded, so just use it
  855             k = check;
  856             CLEAR_PENDING_EXCEPTION;
  857             guarantee((!class_loader.is_null()), "dup definition 
for bootstrap loader?");
  858           }
  859         }

This seems to be a negative test, that we should in fact never get in 
this situation when dealing with the boot loader. But in that case we 
don't need lines 855 and 856 anymore and the code would just collapse to:

  848       if ((class_loader.is_null())) {
  849         if (k == NULL && HAS_PENDING_EXCEPTION
  850           && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {
  851           MutexLocker mu(SystemDictionary_lock, THREAD);
  852           guarantee(find_class(d_hash, name, dictionary) == NULL,
  853                     "dup definition for bootstrap loader?");
  854         }
  855       }

and in that case I'd probably rather see this as an assertion than a 
guarantee, and the whole block can be debug-only.

Thanks,
David
-----

> And, please see in-line comments.
> 
> 
> On 2/8/2018 5:42 PM, David Holmes wrote:
>> Hi Harold,
>>
>> First I'm pretty sure this one can't be pushed until the version bump 
>> arrives in jdk/hs :)
> I hope the version bump arrives soon.
>>
>> On 9/02/2018 6:53 AM, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this JDK-11 change to obsolete the UnsyncloadClass and 
>>> MustCallLoadClassInternal options.  With this change, these options 
>>> are still accepted on the command line but have no affect other than 
>>> to generate these warning messages:
>>>
>>>     Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>     UnsyncloadClass; support was removed in 11.0
>>>
>>>     Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
>>>     MustCallLoadClassInternal; support was removed in 11.0
>>>
>>> Open Webrev: 
>>> http://cr.openjdk.java.net/~hseigel/bug_8184289/webrev/index.html
>>
>> Overall looks good. Tricky to untangle things in the SD especially!
>>
>> src/hotspot/share/classfile/systemDictionary.cpp
>>
>> Looking at this change, and the comment:
>>
>> -       // For UnsyncloadClass only
>>         // If they got a linkageError, check if a parallel class load 
>> succeeded.
>>         // If it did, then for bytecode resolution the specification 
>> requires
>>         // that we return the same result we did for the other thread, 
>> i.e. the
>>         // successfully loaded InstanceKlass
>>         // Should not get here for classloaders that support parallelism
>>         // with the new cleaner mechanism, even with 
>> AllowParallelDefineClass
>>         // Bootstrap goes through here to allow for an extra guarantee 
>> check
>> !       if (UnsyncloadClass || (class_loader.is_null())) {
>>
>> It's not clear why all the "UnsyncLoadClass only" stuff is also being 
>> done for the "Bootstrap" (? bootloader?) case. But in any case the 
>> comment block doesn't read correctly now as this is all, and only, 
>> about the bootstrap case. I'd suggest:
>>
>> -       // For UnsyncloadClass only
>> +       // For bootloader only:
>>         // If they got a linkageError, check if a parallel class load 
>> succeeded.
>>         // If it did, then for bytecode resolution the specification 
>> requires
>>         // that we return the same result we did for the other thread, 
>> i.e. the
>>         // successfully loaded InstanceKlass
>>         // Should not get here for classloaders that support parallelism
>>         // with the new cleaner mechanism, even with 
>> AllowParallelDefineClass
>> -       // Bootstrap goes through here to allow for an extra guarantee 
>> check
> Done.
>>
>>
>> Question: is ClassLoader.loadClassInternal now obsolete as well?
> Yes.  Thanks for pointing that out.  The new webrev contains significant 
> changes needed to remove loadClassInternal.
>>
>> ---
>>
>> src/hotspot/share/runtime/arguments.cpp
>>
>> Is there some reason to leave the expiration version unset? Do you 
>> think the obsoletion warning may be needed for a couple of releases ??
> I figured whoever expires the option can put in the version.
>>
>> ---
>>
>>  test/hotspot/jtreg/runtime/CommandLine/ObsoleteFlagErrorMessage.java
>>
>> You don't need to add anything here. This is not a test of all 
>> obsolete flags, it is just testing some specific handling of obsolete 
>> flags.
> I reverted this change.
> 
> Thanks! Harold
>>
>> Thanks,
>> David
>> -----
>>
>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8184289
>>>
>>> The change was tested with Mach5 tiers 1-5 and the NSK parallel class 
>>> loading tests.  Also, JDK's containing the change were built on all 
>>> Mach5 platforms.
>>>
>>> Thanks, Harold
>>>
>