RFR(M) 8208999 Some use of Klass* should be replaced by InstanceKlass*
Harold David Seigel
harold.seigel at oracle.com
Tue Aug 7 21:11:38 UTC 2018
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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list