RFR (S) Remove SystemDictionary::InitOption enum

[Ioi Lam]

Hi David,

Thanks for the review. Comments below:


On 10/18/18 8:55 PM, David Holmes wrote:
> Hi Ioi,
>
> On 19/10/2018 1:43 PM, Ioi Lam wrote:
>> Hi David,
>>
>> Updated diff here:
>>
>> http://cr.openjdk.java.net/~iklam/jdk12/8212642-remove-sysdict-initoption.v01/delta_1.diff 
>
>
> Updates look good - thanks.
>
> More below ...
>
>>
>>
>> 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. 
>
> But we should never get that far for core classes:
>
>  499   if ((current_class == NULL) ||
>  500       (current_class == new_class) ||
>  501       is_same_class_package(current_class, new_class)) {
>  502     return ACCESS_OK;
>  503   }
>
> as they are in the same package. ??
>
Oh, I missed a few steps: String implements java.io.Serializable. Which 
is a subclass of j.l.Object but a different package.


>> The old code is further convoluted by the fact that 
>> current_class->is_subclass_of(NULL) happily returns false :-(
>
> That seems perfectly reasonable to me. What am I missing? :)
>
>
>> 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.
>
> Ok. I suspect there will be other NULL checks in various places 
> anyway, but this is not a big deal.
>

Or, perhaps the current code is so disciplined that it never touches a 
class that's not yet preloaded (with MagicAccessorImpl being the only 
exception :-) I am sure if you reorder the WK_KLASSES list all hell will 
break.

Thanks
- Ioi




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