RFR: 8285979: G1: G1SegmentedArraySegment::header_size() is incorrect since JDK-8283368

[Thomas Schatzl]

On Sun, 1 May 2022 23:56:55 GMT, Jie Fu <jiefu at openjdk.org> wrote:

> Hi all,
> 
> `G1SegmentedArraySegment::header_size()` is incorrect if `DEFAULT_CACHE_LINE_SIZE <= 32`.
> This bug can be easily reproduced by running `gc/g1` with VM configured and built with `--with-jvm-features=-compiler2`.
> 
> The reason is that there are paddings in the layout of `G1SegmentedArraySegment`.
> 
>   const uint _slot_size;                   // offset 0-byte
>   const uint _num_slots;                   // offset 4-byte
>   const MEMFLAGS _mem_flag;                // offset 8-byte
>     // --- padding 4 bytes
>   G1SegmentedArraySegment* volatile _next; // offset 16-byte
>   uint volatile _next_allocate;            // offset 24
>     // --- padding 4 bytes
>   char* _bottom;                           // offset 32-byte
> 
> 
> So if we calculate `header_size()` like this, it will return 32 bytes when `DEFAULT_CACHE_LINE_SIZE=32`, which should be at least 40 bytes actually.
> 
> static size_t header_size() { return align_up(offset_of(G1SegmentedArraySegment, _bottom) + sizeof(_bottom), DEFAULT_CACHE_LINE_SIZE); }
> 
> 
> Two changes are made in this patch:
> - Fix the implementation of `header_size()`.
> - Fix the layout of `G1SegmentedArraySegment` to eliminate paddings.
> 
> Testing:
> - tier1~3 on Linux/x64, no regression
> - gc/g1 with `--with-jvm-features=-compiler2` on Linux/x64, all passed
> 
> Thanks.
> Best regards,
> Jie

I'll file an RFE to potentially not explicitly store the `_bottom` pointer (only to not forget); there also does not seem to be a need to provide cache line alignment for the payload, regular malloc/new alignment should be just fine.

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1SegmentedArray.hpp line 49:

> 47:   // Do not add class member variables beyond this point
> 48: 
> 49:   static size_t header_size() { return align_up(offset_of(G1SegmentedArraySegment, _bottom) + sizeof(_bottom), DEFAULT_CACHE_LINE_SIZE); }

Suggestion:

  static size_t header_size() { return align_up(sizeof(*this), DEFAULT_CACHE_LINE_SIZE); }


Seems to be better, noticed by Ivan.

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

Marked as reviewed by tschatzl (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8494