Moving the _vtable_len into Klass

[Mikael Gerdin]

Hi Chris,

On 2015-12-16 18:26, Chris Plummer wrote:
> Hi Mikael,
>
> On 12/16/15 3:54 AM, Mikael Gerdin wrote:
>> Hi all,
>>
>> In order to reduce the number of virtual calls in the GC hot path I'd
>> like to propose that the field _vtable_len be moved to class Klass.
>> The reason for requiring access to the vtable length in the GC is that
>> the "nonstatic oop maps" are packed after the v and itables after the
>> actual C++ InstanceKlass.
>>
>> In its current form, the vtable_length() accesor is a pure virtual on
>> Klass, and is implemented in ArrayKlass and InstanceKlass by returning
>> a simple member variable _vtable_len which is present in both
>> ArrayKlass and InstanceKlass.
>> My suggestion is to move the _vtable_len member to Klass and move the
>> accessor for it to Klass as well.
>>
>> This also brings up the issue of
>> InstanceKlass::vtable_length_offset(), which is a very special beast.
>> It returns the offset of the _vtable_len field, in words. This means
>> that the int field _vtable_len must be on a word aligned address in
>> order to work, otherwise we crash at startup in interesting ways.
> Well that seems like fragile programming. The only reason it works right
> now is because _vtable_len comes right after a pointer type. Insert
> another int field before it and it will crash.

Yep, I'd say it's a bug that it's so fragile.

>> I should note that all callers of vtable_length_offset rectify the
>> problem of it being a word offset by multiplying the value by wordSize
>> in order to convert it to a byte offset.
> Yep. I just grep'd and saw the same.
>>
>> Does anyone know if there is a reason for vtable_length_offset to be a
>> word offset?
> Sorry, I don't have any historical perspective here, other than saying
> that specifying size, lengths, and offsets for InstanceKlass related
> stuff seems to all be scaled to HeapWordSize. Just started looking in
> this area recently for the first time myself. BTW, I think it should be
> using wordSize instead of HeapWordSize, although they do both evaluate
> to the same thing.

Yep, I noticed that too.

>>
>> If the offset of the _vtable_len field could be accessed as a byte
>> offset then one would not need to be so careful about the Klass field
>> layout when attempting to fit _vtable_len while simultaneously not
>> increasing the total size of the subclasses of Klass where the field
>> is moved from.
> I'm assuming you've prototyped moving it to just after
> _accumulated_modified_oops, taking advantage of the wasted bytes in the
> word set aside for the jshort _shared_class_path_index field that follows.

In my current variant I've experimented with moving some things around:

diff --git a/src/share/vm/oops/klass.hpp b/src/share/vm/oops/klass.hpp
--- a/src/share/vm/oops/klass.hpp
+++ b/src/share/vm/oops/klass.hpp
@@ -130,18 +130,18 @@
    jint        _modifier_flags;  // Processed access flags, for use by 
Class.getModifiers.
    AccessFlags _access_flags;    // Access flags. The class/interface 
distinction is stored here.

+  TRACE_DEFINE_KLASS_TRACE_ID;
+
    // Biased locking implementation and statistics
    // (the 64-bit chunk goes first, to avoid some fragmentation)
    jlong    _last_biased_lock_bulk_revocation_time;
    markOop  _prototype_header;   // Used when biased locking is both 
enabled and disabled for this type

-  jint     _biased_lock_revocation_count;
-
-  TRACE_DEFINE_KLASS_TRACE_ID;
-
    // vtable length
    int _vtable_len;

+  jint     _biased_lock_revocation_count;
+
    // Remembered sets support for the oops in the klasses.
    jbyte _modified_oops;             // Card Table Equivalent (YC/CMS 
support)
    jbyte _accumulated_modified_oops; // Mod Union Equivalent (CMS support)

>>
>> Regardless of the vtable_length_offset mess, are there any other
>> concerns about making the vtable_length accessor nonvirtual?
>>
>> In order to keep the code consistent and reduce duplication the
>> vtable() accessor could also be moved to Klass, together with
>> start_of_vtable().
>> Unfortunately, there is a circular dependency with InstanceKlass for
>> calculating the vtable start address because the calculation depends
>> on the size of InstanceKlass. However I still think it might be workth
>> consolidating the vtable related methods in Klass since they are in
>> fact part of all subclasses of class.
> So how will you get around the circular dependency?

klass.cpp already includes instanceKlass.hpp (for better or worse) so I 
can simply make the vtable start getter a non-inline member function in 
Klass and it will work out. But I feel a little dirty adding another 
dependency from Klass to InstanceKlass.

/Mikael

>
> Chris
>>
>> Questions/concerns about this suggested change are of course welcome!
>>
>> /Mikael
>