RFR(M) 8208999 Some use of Klass* should be replaced by InstanceKlass*

[Harold David Seigel]

Hi Ioi,

The changes look good.

Thanks, Harold


On 8/7/2018 4:29 PM, Ioi Lam wrote:
> Thanks Coleen!
>
> Could I get a second review?
>
> Thanks
>
> - Ioi
>
>
> On 8/7/18 11:37 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 8/7/18 2:35 PM, Ioi Lam wrote:
>>>
>>>
>>> On 8/7/18 5:30 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 8/7/18 12:31 AM, Ioi Lam wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Thanks for the review. Updated webrev is here:
>>>>>
>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v02/ 
>>>>>
>>>>>
>>>>> More comments below
>>>>>
>>>>>
>>>>> On 8/6/18 3:36 PM, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v01/src/hotspot/share/classfile/systemDictionaryShared.cpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> 1007 return entry != NULL ? entry->instance_klass() : 
>>>>>> (InstanceKlass*)NULL;
>>>>>>
>>>>>> do you need this cast here?
>>>>>>
>>>>> Removed.
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v01/src/hotspot/share/memory/metadataFactory.hpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> + static void free_array(ClassLoaderData* loader_data, const 
>>>>>> Array<T>* data) {
>>>>>>
>>>>>>
>>>>>> But we are modifying the contents of the data (?).  This is for 
>>>>>> deleting secondary_supers?   I think maybe adding the const for 
>>>>>> them might be overkill.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v01/src/hotspot/share/oops/klass.hpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> - Array<Klass*>* _secondary_supers;
>>>>>> + const Array<Klass*>* _secondary_supers;
>>>>>>
>>>>>> I think the improvement outweighs this messy part of this. This 
>>>>>> looks like a difficult thing to change.
>>>>>>
>>>>> I agree. I reverted the "const" for Klass::secondary_supers. I 
>>>>> also removed Array::as_const_array_of() since that will probably 
>>>>> be abused by people who don't read the comments. Instead, I added 
>>>>> this in InstanceKlass::compute_secondary_supers:
>>>>>
>>>>> set_secondary_supers((Array<Klass*>*)(address)interfaces);
>>>>>
>>>>> it's a little ugly but the effects are localized.
>>>>
>>>> I agree, and you can have a short comment above just that part why 
>>>> the cast is needed, and the if statement where you cast them both 
>>>> to address?
>>>>
>>>
>>>
>>> I added these comments. What do you think?
>>>
>>>     // The secondary super list is exactly the same as the 
>>> transitive interfaces, so
>>>     // let's use it instead of making a copy.
>>>     // Redefine classes has to be careful not to delete this!
>>>     // We need the cast because Array<Klass*> is NOT a supertype of 
>>> Array<InstanceKlass*>,
>>>     // (but it's safe to do here because we won't write into 
>>> _secondary_supers from this point on).
>>> set_secondary_supers((Array<Klass*>*)(address)interfaces);
>>>
>>>   if (secondary_supers() != NULL &&
>>>       secondary_supers() != Universe::the_empty_klass_array() &&
>>>       // see comments in compute_secondary_supers about the 
>>> following cast
>>>       (address)(secondary_supers()) != 
>>> (address)(transitive_interfaces()) &&
>>>
>>
>> Yes, this looks good. thank you for adding the comments.
>> Coleen
>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>> The as_const_array_of() will just give someone to something to 
>>>> puzzle over in the future without really increasing safety in my 
>>>> opinion.
>>>>>
>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v01/src/hotspot/share/oops/arrayKlass.cpp.udiff.html 
>>>>>>
>>>>>>
>>>>>> + set_super(Universe::is_bootstrapping() ? (InstanceKlass*)NULL : 
>>>>>> SystemDictionary::Object_klass());
>>>>>>
>>>>>>
>>>>>> Do you need this cast?  Object_klass() returns InstanceKlass - 
>>>>>> all of these do.
>>>>>>
>>>>> Removed.
>>>>
>>>> Thanks!  Thank you for doing this change!
>>>> Coleen
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>>
>>>>>> This looks really good. Thank you for doing this!
>>>>>>
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 8/6/18 5:26 PM, Ioi Lam wrote:
>>>>>>> Hi, please review this clean up
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8208999
>>>>>>> http://cr.openjdk.java.net/~iklam/jdk12/8208999_klass_should_be_instanceklass.v01/ 
>>>>>>>
>>>>>>>
>>>>>>> I changed the following APIs so that we can avoid many 
>>>>>>> unnecessary type casts
>>>>>>> from Klass* -> InstanceKlass*.
>>>>>>>
>>>>>>>     Klass::java_super()                    -> now returns 
>>>>>>> InstanceKlass*
>>>>>>>     InstanceKlass::local_interfaces()      -> 
>>>>>>> Array<InstanceKlass*>*
>>>>>>>     InstanceKlass::transitive_interfaces() -> 
>>>>>>> Array<InstanceKlass*>*
>>>>>>>
>>>>>>> I also modified APIs in SystemSictionary and SystemSictionary to 
>>>>>>> return InstanceKlass*
>>>>>>> where possible.
>>>>>>>
>>>>>>> Most changes are mundane, but here are some notable changes:
>>>>>>>
>>>>>>> 1. To save space, Klass::_secondary_supers (an Array<Klass*>*) 
>>>>>>> may be pointing
>>>>>>>    to the same array as InstanceKlass::_transitive_interfaces (an
>>>>>>>    Array<InstanceKlass*>*).
>>>>>>>
>>>>>>>    To make this happen, I added Array::as_const_array_of().
>>>>>>>    See InstanceKlass::compute_secondary_supers() and array.h.
>>>>>>>
>>>>>>>    I've added enough 'const' and asserts to catch mis-uses of 
>>>>>>> this type
>>>>>>>    of casting. If someone has a better suggestion, please let me 
>>>>>>> know.
>>>>>>>
>>>>>>> 2. I changed Klass::_secondary_supers to 'const Array<Klass*>*' 
>>>>>>> because
>>>>>>>    of #1.
>>>>>>>
>>>>>>>    There are many pointers returned by Klass/InstanceKlass that 
>>>>>>> should be
>>>>>>>    'const', as most callers should read them only without 
>>>>>>> modifying, but that's
>>>>>>>    outside the scope of this RFE.
>>>>>>>
>>>>>>> 3. SystemDictionary::resolve_super_or_fail() currently accepts 
>>>>>>> the format
>>>>>>>    of "Ljava/lang/Object;" for the "super_name" parameter. I am 
>>>>>>> keeping the
>>>>>>>    same behavior in my patch. However, I wonder if this actually 
>>>>>>> correct
>>>>>>>    per JVMLS. I filed JDK-8209030.
>>>>>>>
>>>>>>>    See the call to resolve_instance_class_or_null_helper from 
>>>>>>> resolve_super_or_fail.
>>>>>>>
>>>>>>>
>>>>>>> Testing: hs tiers 1/2/3/4
>>>>>>>
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>
>>>>>
>>>>
>>>
>>
>