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

[coleen.phillimore at oracle.com]

On 2/12/18 1:54 AM, David Holmes wrote:
> 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.

(just to you two).  If this is decided that it's an assert only, my vote 
would be to remove it to simplify the logic here, which is the main 
benefit of removing these options.  It's too special-casey as it is.
Coleen
>
> 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
>>>>
>>