RFR (S) Remove SystemDictionary::InitOption enum

[David Holmes]

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

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

Thanks,
David

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