RFR (T) 8242898: Clean up InstanceKlass::_array_klasses

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Apr 30 10:01:34 UTC 2020



On 4/29/20 8:38 PM, Ioi Lam wrote:
>
>
> On 4/29/20 4:40 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 4/29/20 6:43 PM, Ioi Lam wrote:
>>>
>>>
>>> On 4/29/20 3:38 PM, Ioi Lam wrote:
>>>>
>>>>
>>>> On 4/29/20 2:41 PM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 4/29/20 12:15 PM, Ioi Lam wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> This looks good, but should we also rename array_klasses to 
>>>>>> array_klass, since in most cases we are talking about one klass 
>>>>>> instead of many klasses?
>>>>>
>>>>> It is many classes because they're all linked through 
>>>>> _array_klasses through the higher_dimension(), so it's essentially 
>>>>> more than one.
>>>>>
>>>>
>>>> But this plural usage is inconsistent with other places we use link 
>>>> lists. E.g., klass.hpp
>>>>
>>>>   // First subclass (NULL if none); _subklass->next_sibling() is 
>>>> next one
>>>>   Klass* volatile _subklass;
>>>>   // Sibling link (or NULL); links all subklasses of a klass
>>>>   Klass* volatile _next_sibling;
>>>>   // All klasses loaded by a class loader are chained through these 
>>>> links
>>>>   Klass*      _next_link;
>>>>
>>
>> I see your point but it seems odd to me to call it a single 
>> array_klass.  Sure.  I'll change it and make sure it builds before I 
>> push.
>>
> After discussing with Coleen off-line, I am not insistent on the 
> renaming anymore. I am OK with the current patch as is.

Thanks Ioi,

array_klass() singular can't be used as an accessor because 
Klass::array_klass() singular actually creates the array klass and links 
the dimensions together.  We didn't come up with a mutually agreeable 
alternative name.

Thanks,
Coleen
>
> Thanks
> - Ioi
>
>> Coleen
>>
>>>> and we don't use _supers even if you can use _super to walk all 
>>>> super classes.
>>>>
>>>>   Klass*      _super;
>>>>
>>>>
>>> Also, when you walk "up" the dimensions, it's
>>>
>>> ik->array_klasses()->array_klasses()->array_klasses()->....
>>>
>>> but to walk "down", it's
>>>
>>> nth_dim_obj_array_klass->element_klass()->element_klass()->element_klass()->... 
>>>
>>>
>>> so it's inconsistent even when you are walking the same linked list.
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>> So I'd rather leave the name as is.
>>>>> Thanks,
>>>>> Coleen
>>>>>>
>>>>>> The plural makes sense only in places like this:
>>>>>>
>>>>>>
>>>>>> 1625 void InstanceKlass::array_klasses_do(void f(Klass* k, 
>>>>>> TRAPS), TRAPS) {
>>>>>> 1626   if (array_klass() != NULL)
>>>>>> 1627     array_klass()->array_klasses_do(f, THREAD);
>>>>>> 1628 }
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>> On 4/29/20 6:06 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 4/29/20 8:44 AM, David Holmes wrote:
>>>>>>>> Hi Coleen,
>>>>>>>>
>>>>>>>> On 29/04/2020 10:11 pm, coleen.phillimore at oracle.com wrote:
>>>>>>>>> Summary: Use more specific ObjArrayKlass type.
>>>>>>>>>
>>>>>>>>> This avoids some casts.  Tested with tier1-3.
>>>>>>>>>
>>>>>>>>> open webrev at 
>>>>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8242898.01/webrev
>>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8242898
>>>>>>>>
>>>>>>>> This looks fine to me.
>>>>>>>>
>>>>>>>> I have to wonder why this was not defined this way all along?
>>>>>>>
>>>>>>> It was likely klassOop and stayed that way until Ioi noticed.
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Coleen
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list