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

[coleen.phillimore at oracle.com]

On 2/9/18 2:44 PM, 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
>
> 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.

The odd thing about this block is that it reads like it no longer 
applies.   Logically it was executed with NULL class loader but I wonder 
if it needed to be or if the whole code block can be removed?   Don't we 
have the new "cleaner" mechanism?

Other than that, I think the change looks good and helps with following 
logic through this class loading code.

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

I think you should add 12 to it, so we'll be sure to make it go away.

Thanks,
Coleen

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