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

[Ioi Lam]

Hi Harold,

Thanks for the review!

- Ioi

On 8/7/18 2:11 PM, Harold David Seigel wrote:
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>