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

[Mikael Gerdin]

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.

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


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