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