RFR: 8334220: Optimize Klass layout after JDK-8180450 [v2]

[Xiaolong Peng]

On Tue, 2 Jul 2024 17:04:40 GMT, Xiaolong Peng <xpeng at openjdk.org> wrote:

>> src/hotspot/share/oops/klass.hpp line 166:
>> 
>>> 164:   uintx    _bitmap;
>>> 165: 
>>> 166:   static uint8_t compute_hash_slot(Symbol* s);
>> 
>> We don't usually put functions in with nonstatic member declarations.  Since you moved hash_slot, can you move this function to the first private section where hash_insert is?
>> 
>> Where you moved hash_slot looks fine.  Doesn't look like it will have negative cache effects, unless it needs to be on a cache line with _bitmap?
>
> Thanks Coleen! Good catch! 
> I have checked the cacheline boundaries  with pahole, both are in same cache line after moving, that shouldn't be a concern.
> But yes, they are related fields and should be stay together. Also checked the layer after moving both  _bitmap and _hash_slot, it looks good, I'll update the PR:
> 
> protected:
> 
> 	jint                       _layout_helper;       /*     8     4 */
> 	const enum KlassKind       _kind;                /*    12     4 */
> 	jint                       _modifier_flags;      /*    16     4 */
> 	juint                      _super_check_offset;  /*    20     4 */
> 	class Symbol *             _name;                /*    24     8 */
> 	class Klass *              _secondary_super_cache; /*    32     8 */
> 	class Array<Klass*> *      _secondary_supers;    /*    40     8 */
> 	class Klass *              _primary_supers[8];   /*    48    64 */
> 	/* --- cacheline 1 boundary (64 bytes) was 48 bytes ago --- */
> 	class OopHandle           _java_mirror;          /*   112     8 */
> 	class Klass *              _super;               /*   120     8 */
> 	/* --- cacheline 2 boundary (128 bytes) --- */
> 	volatile class Klass *     _subklass;            /*   128     8 */
> 	volatile class Klass *     _next_sibling;        /*   136     8 */
> 	class Klass *              _next_link;           /*   144     8 */
> 	class ClassLoaderData *    _class_loader_data;   /*   152     8 */
> 	int                        _vtable_len;          /*   160     4 */
> 	class AccessFlags         _access_flags;         /*   164     4 */
> 	traceid                    _trace_id;            /*   168     8 */
> 	uintx                      _bitmap;              /*   176     8 */
> 	uint8_t                    _hash_slot;           /*   184     1 */
> 
> 	/* XXX 1 byte hole, try to pack */
> 
> 	s2                         _shared_class_path_index; /*   186     2 */
> 	u2                         _shared_class_flags;  /*   188     2 */
> 
> 	/* XXX 2 bytes hole, try to pack */
> 
> 	/* --- cacheline 3 boundary (192 bytes) --- */
> 	int                        _archived_mirror_index; /*   192     4 */

I have updated PR to reflect what we have discussed, thanks!

>> src/hotspot/share/oops/klass.hpp line 176:
>> 
>>> 174:   JFR_ONLY(DEFINE_TRACE_ID_FIELD;)
>>> 175:   uint8_t  _hash_slot;
>>> 176:   DEFINE_PAD_MINUS_SIZE(1, 4, sizeof(uint8_t)); //3 bytes padding after 1 byte _hash_slot for better layout
>> 
>> How does this help? Doesn't the compiler add this padding?
>
> Compiler doesn't seem to add padding, there will be two smaller holes after _hash_slot, here is what I got from gdb:
> 
> /* 0x00b8      |  0x0001 */    uint8_t _hash_slot;
>                              private:
> /* XXX  1-byte hole      */
> /* 0x00ba      |  0x0002 */    s2 _shared_class_path_index;
> /* 0x00bc      |  0x0002 */    u2 _shared_class_flags;
> /* XXX  2-byte hole      */
> /* 0x00c0      |  0x0004 */    int _archived_mirror_index;
> 
> 
> I think we can remove it to avoid confusion, I doesn't really change the cache line, here is output from ```pahole``` with cacheline boundaries:
> With padding:
> 
> protected:
> 
> 	jint                       _layout_helper;       /*     8     4 */
> 	const enum KlassKind       _kind;                /*    12     4 */
> 	jint                       _modifier_flags;      /*    16     4 */
> 	juint                      _super_check_offset;  /*    20     4 */
> 	class Symbol *             _name;                /*    24     8 */
> 	class Klass *              _secondary_super_cache; /*    32     8 */
> 	class Array<Klass*> *      _secondary_supers;    /*    40     8 */
> 	class Klass *              _primary_supers[8];   /*    48    64 */
> 	/* --- cacheline 1 boundary (64 bytes) was 48 bytes ago --- */
> 	class OopHandle           _java_mirror;          /*   112     8 */
> 	class Klass *              _super;               /*   120     8 */
> 	/* --- cacheline 2 boundary (128 bytes) --- */
> 	volatile class Klass *     _subklass;            /*   128     8 */
> 	volatile class Klass *     _next_sibling;        /*   136     8 */
> 	class Klass *              _next_link;           /*   144     8 */
> 	class ClassLoaderData *    _class_loader_data;   /*   152     8 */
> 	uintx                      _bitmap;              /*   160     8 */
> 	int                        _vtable_len;          /*   168     4 */
> 	class AccessFlags         _access_flags;         /*   172     4 */
> 	traceid                    _trace_id;            /*   176     8 */
> 	uint8_t                    _hash_slot;           /*   184     1 */
> 	char                       _pad_buf1[3];         /*   185     3 */
> 	s2                         _shared_class_path_index; /*   188     2 */
> 	u2                         _shared_class_flags;  /*   190     2 */
> 	/* --- cacheline 3 boundary (192 bytes) --- */
> 	int                        _archived_mirror_index; /*   192     4 */
> 
> 
> w/o padding:
> 
> protected:
> 
> 	jint                       _layout_helper;       /*     8     4 */
> 	const enum KlassKind       _kind;                /*    12   ...

I have removed the padding from code, thanks.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19958#discussion_r1662905230
PR Review Comment: https://git.openjdk.org/jdk/pull/19958#discussion_r1662907038