RFR(M) 8148047: Move the vtable length field to Klass

Mikael Gerdin mikael.gerdin at oracle.com
Wed Jan 27 12:43:12 UTC 2016


Hi Chris,

On 2016-01-25 18:02, Chris Plummer wrote:
> Hi Mikael,
>
> On 1/25/16 6:34 AM, Mikael Gerdin wrote:
>> Hi Chris,
>>
>> On 2016-01-22 20:21, Chris Plummer wrote:
>>> Hi Mikael,
>>>
>>> On 1/22/16 7:49 AM, Mikael Gerdin wrote:
>>>> Hi all,
>>>>
>>>> Here's the second part of the set of changes to move most of the
>>>> vtable related code to Klass.
>>>>
>>>> This change consists of the following parts:
>>>> * Move the field _vtable_len to Klass, making its accessor nonvirtual.
>>>>  -> Ensure that this does not result in any footprint regression by
>>>> moving TRACE_DEFINE_KLASS_TRACE_ID in Klass and _itable_len in
>>>> InstanceKlass to fill out alignment gaps.
>>> I also double checked this, including with minimal VM. The sizeof(Klass)
>>> remains unchanged.
>>
>> Thanks for verifying.
>>
>>>
>>> And your relocation of InstanceKlass::_itable_len should result in
>>> InstanceKlass being 8 bytes smaller on 64-bit, although I did not double
>>> check that.
>>>>  -> Move vtable_length_offset to Klass. Move vtable_start_offset to
>>>> Klass to keep the code consistent. vtable_start_offset depends on the
>>>> size of InstanceKlass and must therefore be defined outside of
>>>> klass.hpp.
>>> The abstraction has kind of gone awry:
>>>
>>>   662 ByteSize Klass::vtable_start_offset() {
>>>   663   return in_ByteSize(InstanceKlass::header_size() * wordSize);
>>>   664 }
>>>
>>> Note the reference to InstanceKlass. I understand why you are doing
>>> this. All Klass subclasses have their sizes padded out to the size of
>>> InstanceKlass so the vtable is always in the same place. I was just
>>> wondering if there is a cleaner way. I guess the abstraction was broken
>>> before your changes, but in a different place.  We used to call
>>> InstanceKlass::vtable_start_offset() even for array types. Now you
>>> instead call Klass::vtable_start_offset(), and it does the dirty deed of
>>> referencing InstanceKlass. I guess I'm ok with it. Just wanted to point
>>> it out in case you had more thoughts.
>>
>> I agree that it's not the nicest abstraction, I could "hide" it in a
>> static field in Klass or Universe or something, but that's just
>> sweeping the problem under the rug.
>>
>> On a slightly related topic, I did consider attempting to add some
>> sort of assertion that InstanceKlass is indeed the largest of all
>> subclasses of Klass, otherwise this code would be broken. For example
>> if someone had the idea of adding a sufficiently large field to a
>> subclass of InstanceKlass then the vtable would conflict with that field.
> I think you meant add a field to a subclass of Klass, not InstanceKlass.
>>
>> One approach would be to have a protected Klass constructor which
>> takes a template parameter T, where subclasses pass "themselves" and
>> the Klass constructor verifies that sizeof(T) <= sizeof(InstanceKlass)
>> (with or without word size scaling).
> There's already a check in place for ArrayKlass subclasses, which do
> something similar by passing in their own size:
>
> int ArrayKlass::static_size(int header_size) {
>    // size of an array klass object
>    assert(header_size <= InstanceKlass::header_size(), "bad header size");
>    // If this assert fails, see comments in base_create_array_klass.
>    header_size = InstanceKlass::header_size();
>    int vtable_len = Universe::base_vtable_size();
>    int size = header_size + vtable_len;
>    return align_object_size(size);
> }

Oh, I hadn't' noticed that.
That's good at least for ArrayKlass then.

Thanks for the info
/Mikael

>
> cheers,
>
> Chris
>>
>>
>>>
>>>>
>>>> * Update all locations to refer to Klass::vtable_{length,start}_offset
>>>> instead of InstanceKlass.
>>> Looks good.
>>>>
>>>> * Modify SA to look for _vtable_len in Klass.
>>> Looks good.
>>>>
>>>> * Rename CompilerToVM::Data::InstanceKlass_vtable_{length,start}_offset
>>>> and
>>>> jdk.vm.ci.hotspot.HotSpotVMConfig.instanceKlassVtable{Length,Start}Offset
>>>>
>>>> to properly represent where the offsets are coming from.
>>> Looks good.
>>>>
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~mgerdin/8148047/webrev.0/
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8148047
>>>>
>>>> Testing: JPRT on Oracle supported platforms.
>>>>
>>>> As in the previous change I've updated the PPC64 and AARCH64 ports but
>>>> I have not tested the changes. Build and test feedback from porters is
>>>> most welcome!
>>>
>>> Cheers,
>>
>> Thanks for the review, Chris.
>> /Mikael
>>
>>>
>>> Chris
>>>>
>>>> Thanks
>>>> /Mikael
>>>
>>
>



More information about the hotspot-dev mailing list