8334220: Optimize Klass layout after JDK-8180450

[pengxiaolong]

Hi all,
This PR is created to optimize the layout of Klass in hotspot, after JDK-8180450 the layout of Klsss seems broken, there are 3 holes, they are caused by alignment issue introduced by the 1 byte _hash_slot.

(gdb) ptype /ox Klass
/* offset      |    size */  type = class Klass : public Metadata {
                             public:
                               static const uint KLASS_KIND_COUNT;
                             protected:
/* 0x000c      |  0x0004 */    jint _layout_helper;
/* 0x0010      |  0x0004 */    const enum Klass::KlassKind _kind;
/* 0x0014      |  0x0004 */    jint _modifier_flags;
/* 0x0018      |  0x0004 */    juint _super_check_offset;
/* XXX  4-byte hole      */
/* 0x0020      |  0x0008 */    class Symbol *_name;
/* 0x0028      |  0x0008 */    class Klass *_secondary_super_cache;
/* 0x0030      |  0x0008 */    class Array<Klass*> *_secondary_supers;
/* 0x0038      |  0x0040 */    class Klass *_primary_supers[8];
/* 0x0078      |  0x0008 */    class OopHandle {
                                 private:
/* 0x0078      |  0x0008 */        class oop *_obj;

                                   /* total size (bytes):    8 */
                               } _java_mirror;
/* 0x0080      |  0x0008 */    class Klass *_super;
/* 0x0088      |  0x0008 */    class Klass * volatile _subklass;
/* 0x0090      |  0x0008 */    class Klass * volatile _next_sibling;
/* 0x0098      |  0x0008 */    class Klass *_next_link;
/* 0x00a0      |  0x0008 */    class ClassLoaderData *_class_loader_data;
/* 0x00a8      |  0x0008 */    uintx _bitmap;
/* 0x00b0      |  0x0001 */    uint8_t _hash_slot;
/* XXX  3-byte hole      */
/* 0x00b4      |  0x0004 */    int _vtable_len;
/* 0x00b8      |  0x0004 */    class AccessFlags {
                                 private:
/* 0x00b8      |  0x0004 */        jint _flags;

                                   /* total size (bytes):    4 */
                               } _access_flags;
/* XXX  4-byte hole      */
/* 0x00c0      |  0x0008 */    traceid _trace_id;
                             private:
/* 0x00c8      |  0x0002 */    s2 _shared_class_path_index;
/* 0x00ca      |  0x0002 */    u2 _shared_class_flags;
/* 0x00cc      |  0x0004 */    int _archived_mirror_index;
                             public:
                               static const int SECONDARY_SUPERS_TABLE_SIZE;
                               static const int SECONDARY_SUPERS_TABLE_MASK;
                               static const uintx SECONDARY_SUPERS_BITMAP_EMPTY;
                               static const uintx SECONDARY_SUPERS_BITMAP_FULL;
                               static const int _lh_neutral_value;
                               static const int _lh_instance_slow_path_bit;
                               static const int _lh_log2_element_size_shift;
                               static const int _lh_log2_element_size_mask;
                               static const int _lh_element_type_shift;
                               static const int _lh_element_type_mask;
                               static const int _lh_header_size_shift;
                               static const int _lh_header_size_mask;
                               static const int _lh_array_tag_bits;
                               static const int _lh_array_tag_shift;
                               static const int _lh_array_tag_obj_value;
                               static const unsigned int _lh_array_tag_type_value;

                               /* total size (bytes):  208 */
                             }

As Aleksey suggested, moving _hash_slot to somewhere later could solve the alignments issue, I have tested it it works well, but causes 2 smaller holes in the private fields which could be solved by padding.

Layout after moving _hash_slot w/o padding

/* offset      |    size */  type = class Klass : public Metadata {
                             public:
                               static const uint KLASS_KIND_COUNT;
                             protected:
/* 0x0008      |  0x0004 */    jint _layout_helper;
/* 0x000c      |  0x0004 */    const enum Klass::KlassKind _kind;
/* 0x0010      |  0x0004 */    jint _modifier_flags;
/* 0x0014      |  0x0004 */    juint _super_check_offset;
/* 0x0018      |  0x0008 */    class Symbol *_name;
/* 0x0020      |  0x0008 */    class Klass *_secondary_super_cache;
/* 0x0028      |  0x0008 */    class Array<Klass*> *_secondary_supers;
/* 0x0030      |  0x0040 */    class Klass *_primary_supers[8];
/* 0x0070      |  0x0008 */    class OopHandle {
                                 private:
/* 0x0070      |  0x0008 */        oop *_obj;

                                   /* total size (bytes):    8 */
                               } _java_mirror;
/* 0x0078      |  0x0008 */    class Klass *_super;
/* 0x0080      |  0x0008 */    class Klass * volatile _subklass;
/* 0x0088      |  0x0008 */    class Klass * volatile _next_sibling;
/* 0x0090      |  0x0008 */    class Klass *_next_link;
/* 0x0098      |  0x0008 */    class ClassLoaderData *_class_loader_data;
/* 0x00a0      |  0x0008 */    uintx _bitmap;
/* 0x00a8      |  0x0004 */    int _vtable_len;
/* 0x00ac      |  0x0004 */    class AccessFlags {
                                 private:
/* 0x00ac      |  0x0004 */        jint _flags;

                                   /* total size (bytes):    4 */
                               } _access_flags;
/* 0x00b0      |  0x0008 */    traceid _trace_id;
/* 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;
                             public:
                               static const int SECONDARY_SUPERS_TABLE_SIZE;
                               static const int SECONDARY_SUPERS_TABLE_MASK;
                               static const uintx SECONDARY_SUPERS_BITMAP_EMPTY;
                               static const uintx SECONDARY_SUPERS_BITMAP_FULL;
                               static const int _lh_neutral_value;
                               static const int _lh_instance_slow_path_bit;
                               static const int _lh_log2_element_size_shift;
                               static const int _lh_log2_element_size_mask;
                               static const int _lh_element_type_shift;
                               static const int _lh_element_type_mask;
                               static const int _lh_header_size_shift;
                               static const int _lh_header_size_mask;
                               static const int _lh_array_tag_bits;
                               static const int _lh_array_tag_shift;
                               static const int _lh_array_tag_obj_value;
                               static const unsigned int _lh_array_tag_type_value;
/* XXX  4-byte padding   */

                               /* total size (bytes):  200 */
                             }

Layout after moving _hash_slot with padding:

/* offset      |    size */  type = class Klass : public Metadata {
                             public:
                               static const uint KLASS_KIND_COUNT;
                             protected:
/* 0x0008      |  0x0004 */    jint _layout_helper;
/* 0x000c      |  0x0004 */    const enum Klass::KlassKind _kind;
/* 0x0010      |  0x0004 */    jint _modifier_flags;
/* 0x0014      |  0x0004 */    juint _super_check_offset;
/* 0x0018      |  0x0008 */    class Symbol *_name;
/* 0x0020      |  0x0008 */    class Klass *_secondary_super_cache;
/* 0x0028      |  0x0008 */    class Array<Klass*> *_secondary_supers;
/* 0x0030      |  0x0040 */    class Klass *_primary_supers[8];
/* 0x0070      |  0x0008 */    class OopHandle {
                                 private:
/* 0x0070      |  0x0008 */        oop *_obj;

                                   /* total size (bytes):    8 */
                               } _java_mirror;
/* 0x0078      |  0x0008 */    class Klass *_super;
/* 0x0080      |  0x0008 */    class Klass * volatile _subklass;
/* 0x0088      |  0x0008 */    class Klass * volatile _next_sibling;
/* 0x0090      |  0x0008 */    class Klass *_next_link;
/* 0x0098      |  0x0008 */    class ClassLoaderData *_class_loader_data;
/* 0x00a0      |  0x0008 */    uintx _bitmap;
/* 0x00a8      |  0x0004 */    int _vtable_len;
/* 0x00ac      |  0x0004 */    class AccessFlags {
                                 private:
/* 0x00ac      |  0x0004 */        jint _flags;

                                   /* total size (bytes):    4 */
                               } _access_flags;
/* 0x00b0      |  0x0008 */    traceid _trace_id;
/* 0x00b8      |  0x0001 */    uint8_t _hash_slot;
/* 0x00b9      |  0x0003 */    char _pad_buf1[3];
                             private:
/* 0x00bc      |  0x0002 */    s2 _shared_class_path_index;
/* 0x00be      |  0x0002 */    u2 _shared_class_flags;
/* 0x00c0      |  0x0004 */    int _archived_mirror_index;
                             public:
                               static const int SECONDARY_SUPERS_TABLE_SIZE;
                               static const int SECONDARY_SUPERS_TABLE_MASK;
                               static const uintx SECONDARY_SUPERS_BITMAP_EMPTY;
                               static const uintx SECONDARY_SUPERS_BITMAP_FULL;
                               static const int _lh_neutral_value;
                               static const int _lh_instance_slow_path_bit;
                               static const int _lh_log2_element_size_shift;
                               static const int _lh_log2_element_size_mask;
                               static const int _lh_element_type_shift;
                               static const int _lh_element_type_mask;
                               static const int _lh_header_size_shift;
                               static const int _lh_header_size_mask;
                               static const int _lh_array_tag_bits;
                               static const int _lh_array_tag_shift;
                               static const int _lh_array_tag_obj_value;
                               static const unsigned int _lh_array_tag_type_value;
/* XXX  4-byte padding   */

                               /* total size (bytes):  200 */
                             }

Best,
Xiaolong.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8334220: Optimize Klass layout after JDK-8180450 (Sub-task - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19958/head:pull/19958
$ git checkout pull/19958

Update a local copy of the PR:
$ git checkout pull/19958
$ git pull https://git.openjdk.org/jdk.git pull/19958/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19958

View PR using the GUI difftool:
$ git pr show -t 19958

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19958.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back xpeng! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@pengxiaolong This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8334220: Optimize Klass layout after JDK-8180450

Reviewed-by: coleenp, stuefe, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

• f7af450: 8335110: Fix instruction name and API spec inconsistencies in CodeBuilder
• 8a664a4: 8334734: Remove specialized readXxxEntry methods from ClassReader
• 3a2d426: 8334726: Remove accidentally exposed individual methods from Class-File API
• f187c92: 8335370: Fix -Wzero-as-null-pointer-constant warning in jvmti_common.hpp
• 1ef34c1: 8335475: ClassBuilder incorrectly calculates max_locals in some cases
• 27982c8: 8327854: Test java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpStatefulTest.java failed with RuntimeException
• a347957: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754
• 153b12b: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers
• 685e587: 8334816: compiler/c2/irTests/TestIfMinMax.java fails after 8334629
• dd74e7f: 8335147: Serial: Refactor TenuredGeneration::promote
• ... and 14 more: https://git.openjdk.org/jdk/compare/bb18498d71dddf49db9bdfac886aed9ae123651d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp, @tstuefe, @dholmes-ora) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[openjdk]

@pengxiaolong The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (ce1560c1)
• 00: Full (00fbff70)

[coleenp]

I have a couple of questions about this and a request. Thanks.

[tstuefe]

Ok.

I have checked the cacheline boundaries with pahole, both are in same cache line after moving, that shouldn't be a concern.

Note that in current class space, Klass does usually not start at a cache line boundary.

[pengxiaolong]

I have a couple of questions about this and a request. Thanks.

Thanks so much for the suggestions! Do you have other concerns on the new revision?

[pengxiaolong]

Ok.

I have checked the cacheline boundaries with pahole, both are in same cache line after moving, that shouldn't be a concern.

Note that in current class space, Klass does usually not start at a cache line boundary.

Thank you Thomas for the review and reminding! The whole point is to compact the layout of Klass, which is instantiated often at runtime, more compact will benefit the footprint of cachelines, although the improvement won't be significant.

[coleenp]

Yes, this looks good.

[pengxiaolong]

Yes, this looks good.

Thank you, appreciate it!

/integrate

[openjdk]

@pengxiaolong
Your change (at version ce1560c) is now ready to be sponsored by a Committer.

[dholmes-ora]

/sponsor

[openjdk]

Going to push as commit f9b4ea1.
Since your change was applied there have been 24 commits pushed to the master branch:

• f7af450: 8335110: Fix instruction name and API spec inconsistencies in CodeBuilder
• 8a664a4: 8334734: Remove specialized readXxxEntry methods from ClassReader
• 3a2d426: 8334726: Remove accidentally exposed individual methods from Class-File API
• f187c92: 8335370: Fix -Wzero-as-null-pointer-constant warning in jvmti_common.hpp
• 1ef34c1: 8335475: ClassBuilder incorrectly calculates max_locals in some cases
• 27982c8: 8327854: Test java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpStatefulTest.java failed with RuntimeException
• a347957: 8335291: Problem list all SA core file tests on macosx-aarch64 due to JDK-8318754
• 153b12b: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers
• 685e587: 8334816: compiler/c2/irTests/TestIfMinMax.java fails after 8334629
• dd74e7f: 8335147: Serial: Refactor TenuredGeneration::promote
• ... and 14 more: https://git.openjdk.org/jdk/compare/bb18498d71dddf49db9bdfac886aed9ae123651d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora @pengxiaolong Pushed as commit f9b4ea1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.