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

[harold seigel]

Hi Coleen,

Thanks for reviewing this.  Please see comments in-line.


On 2/9/2018 3:50 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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?
I don't know the answer to this.  I'll look into it further.
>
> Other than that, I think the change looks good and helps with 
> following logic through this class loading code.
Thanks!
>
>>>
>>>
>>> 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.
Okay. Will do.

Thanks,  Harold
>
> 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
>>>>
>>
>