RFR(M) 8208999 Some use of Klass* should be replaced by InstanceKlass*
Ioi Lam
ioi.lam at oracle.com
Tue Aug 7 20:29:41 UTC 2018
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