RFR (S) Remove SystemDictionary::InitOption enum

[Ioi Lam]

Hi David,

Updated diff here:

http://cr.openjdk.java.net/~iklam/jdk12/8212642-remove-sysdict-initoption.v01/delta_1.diff


On 10/18/18 6:14 PM, David Holmes wrote:
> Hi Ioi,
>
> Please include the bug number in RFR subject - thanks.
>
> On 19/10/2018 2:25 AM, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8212642
>> http://cr.openjdk.java.net/~iklam/jdk12/8212642-remove-sysdict-initoption.v01/ 
>>
>>
>> SystemDictionary::InitOption is no longer needed because we don't 
>> support
>> multiple version of the JDK class libraries anymore. All classes 
>> preloaded
>> by SystemDictionary must exist.
>
> In systemDictionary.cpp/hpp:
>
> 1974 bool SystemDictionary::resolve_wk_klass(WKID id, int init_opt, 
> TRAPS) {
>
> The init_opt parameter is now unused.
>

Removed.

> ---
>
> src/hotspot/share/classfile/systemDictionary.hpp
>
>  96 // Each well-known class has a short klass name (like object_klass),
>  97 // a vmSymbol name (like java_lang_Object), and a flag word
>  98 // that makes some minor distinctions, like whether the klass
>  99 // is preloaded, optional, release-specific, etc.
>
> The comment is no longer accurate/correct.
>

Fixed.

> ---
>
> src/hotspot/share/runtime/reflection.cpp
>
> Why is MagicAccessorImpl still treated as optional?
>
>
Reflection::verify_class_access() is called for every class that has a 
super class. So it gets called for String, which is loaded immediately 
after Object. At this point, MagicAccessorImpl is not loaded yet, so the 
old code marks it as "optional" to get around this. The old code is 
further convoluted by the fact that current_class->is_subclass_of(NULL) 
happily returns false :-(

I think the new code expresses what we want -- if MagicAccessorImpl is 
not loaded yet, obviously none of its subclasses are loaded, so there's 
nothing to check.

>> The JVMCI classes were declared using InitOption::Jvmci, so that they 
>> are loaded
>> only when EnableJVMCI==true. However, this is rather convoluted and
>> I just added a few null checks instead.
>
> Can't help but think check_klass is redundant now. It would simplify 
> things if it were removed, then you wouldn't need to check is_loaded. 
> The only reason for the check is to avoid the NULL-check in the 
> assertion in check_klass when called from name(). The JVMCI code 
> either has to handle the NULL anyway, or else is never called unless 
> it can't be NULL.
>
check_klass() probably has a purpose -- before all the preloaded classes 
are resolved, someone may call SystemDictionary::xxx_klass() and store 
the result somewhere, and will get a crash only when the stored result 
is used later.

A lot of code is executed inside 
SystemDictionary::resolve_preloaded_classes -- class file parsing, 
JVMTI, verification if requested by the user, etc. So I think it's 
better to preserve this check.

Thanks
- Ioi
> Thanks,
> David
>
>> Tested using hs tiers 1-5
>>
>> Thanks
>> - Ioi