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

[coleen.phillimore at oracle.com]

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?

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