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

[Mikael Gerdin]

Hi Kim,


On 2016-02-01 14:41, Kim Barrett wrote:
>> On Jan 22, 2016, at 10:49 AM, Mikael Gerdin <mikael.gerdin at oracle.com> 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.
>> -> 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.
>>
>> * Update all locations to refer to Klass::vtable_{length,start}_offset instead of InstanceKlass.
>>
>> * Modify SA to look for _vtable_len in Klass.
>>
>> * 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.
>>
>>
>> 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!
>
> Looks good.
>
> A couple of surprises noted.
>
> ------------------------------------------------------------------------------
> src/share/vm/oops/arrayKlass.cpp
>    85 ArrayKlass::ArrayKlass(Symbol* name) :
> src/share/vm/oops/instanceKlass.cpp
>   211 InstanceKlass::InstanceKlass(const ClassFileParser& parser, unsigned kind) :
>
> [Pre-existing]
>
> I was surprised by the indentation of the bodies of these
> constructors. I would have placed the opening brace on its own line
> (to separate the init-list from the body) and indented the body
> normally.  I'm guessing there are more like this, so perhaps I should
> get over my surprise.

There are indeed some weird things in the deepest, darkest corners :)

>
> ------------------------------------------------------------------------------
> src/share/vm/oops/arrayKlass.cpp
>    91     set_vtable_length(Universe::base_vtable_size());
> src/share/vm/oops/instanceKlass.cpp
>   216     set_vtable_length(parser.vtable_size());
>
> [Sort of pre-existing]
> I was surprised that the Klass constructor left the new _vtable_len
> field uninitialized, with assignment done in subclasses. I was
> expecting the Klass constructor to be called with arguments that would
> be used to initialize various fields, with no need for the setter
> functions. But what's in the webrev seems to be of the general style
> used in this vicinity.  Perhaps a followup cleanup is called for?

Yeah, the fact that Klass only has a no-args constructor surprised me as 
well. I can file a followup cleanup but I don't really have the time to 
do further cleanups in this area at this time.


>
> ------------------------------------------------------------------------------
>
> I agree with Chris that something has gone awry with
>
> 662 ByteSize Klass::vtable_start_offset() {
> 663   return in_ByteSize(InstanceKlass::header_size() * wordSize);
> 664 }
>
> But I don't have a suggestion for improvement right now.

Ok.
Thanks for the review, Kim.

/Mikael

>
> ------------------------------------------------------------------------------
>