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

[Chris Plummer]

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);
}

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
>>
>