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

Ioi Lam ioi.lam at oracle.com
Tue Aug 7 04:31:59 UTC 2018


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.

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



More information about the hotspot-runtime-dev mailing list