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

[harold seigel]

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