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

[David Holmes]

Hi Harold,

First I'm pretty sure this one can't be pushed until the version bump 
arrives in jdk/hs :)

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


Question: is ClassLoader.loadClassInternal now obsolete as well?

---

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

---

  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.	

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
>