RFR(M) 8208999 Some use of Klass* should be replaced by InstanceKlass*
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Aug 7 18:37:15 UTC 2018
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