RFR: 8139457: Array bases are aligned at HeapWord granularity [v23]

[Stefan Karlsson]

On Tue, 14 Feb 2023 08:33:23 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> See [JDK-8139457](https://bugs.openjdk.org/browse/JDK-8139457) for details.
>> 
>> Basically, when running with -XX:-UseCompressedClassPointers, arrays will have a gap between the length field and the first array element, because array elements will only start at word-aligned offsets. This is not necessary for smaller-than-word elements.
>> 
>> Also, while it is not very important now, it will become very important with Lilliput, which eliminates the Klass field and would always put the length field at offset 8, and leave a gap between offset 12 and 16.
>> 
>> Testing:
>>  - [x] runtime/FieldLayout/ArrayBaseOffsets.java (x86_64, x86_32, aarch64, arm, riscv, s390)
>>  - [x] bootcycle (x86_64, x86_32, aarch64, arm, riscv, s390)
>>  - [x] tier1 (x86_64, x86_32, aarch64, riscv)
>>  - [x] tier2 (x86_64, aarch64, riscv)
>>  - [x] tier3 (x86_64, riscv)
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Clarify comment on arrayOopDesc::max_array_length()

Changes requested by stefank (Reviewer).

src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp line 202:

> 200:     assert(is_aligned(hdr_size_in_bytes, BytesPerInt), "must be 32-bit-aligned");
> 201:     strw(zr, Address(obj, hdr_size_in_bytes));
> 202:     hdr_size_in_bytes += BytesPerInt;

With this change `hdr_size_in_bytes` stops containing the header size in bytes. This could be confusing to readers and a potential source for bugs in the future. This comment applies to more places where we adjust the header size / base offset in the code.

Given that this is compiler code I let you and the compiler maintainers decide if this should be changed.

src/hotspot/cpu/arm/c1_MacroAssembler_arm.cpp line 166:

> 164: void C1_MacroAssembler::allocate_array(Register obj, Register len,
> 165:                                        Register tmp1, Register tmp2, Register tmp3,
> 166:                                        int header_size_in_bytes, int element_size,

Other platforms call this parameter base_offset_in_bytes. I looked at the code below, but couldn't convince myself that the MinOjbAlignmentInBytesMask usages were correct. Maybe they are because this is 32-bit code, but could be worth taking a closer look.

src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 303:

> 301:   }
> 302:   add2reg(arr_size, base_offset_in_bytes + MinObjAlignmentInBytesMask); // Add space for header & alignment.
> 303:   z_nill(arr_size, (~MinObjAlignmentInBytesMask) & 0xffff);            // Align array size.

Comments became unaligned

src/hotspot/share/gc/shared/collectedHeap.cpp line 426:

> 424:   int payload_start_in_words = payload_start / HeapWordSize;
> 425:   Copy::fill_to_words(start + payload_start_in_words,
> 426:                       words - payload_start_in_words, value);

`start` refers to an address and `payload_start` refers to an offset. It would be good to not reuse the word `start` for to different types. Maybe rename to `payload_offset`?

src/hotspot/share/gc/shared/jvmFlagConstraintsGC.cpp line 340:

> 338:     return JVMFlag::VIOLATES_CONSTRAINT;
> 339:   }
> 340:   if (value > ((uint64_t)ThreadLocalAllocBuffer::max_size() * HeapWordSize)) {

Was this change needed because _filler_array_max_size was changed and max_tlab_size uses it? Could you motivate why this is needed or add an assert that we don't overflow size_t.

I see that we have another place where we don't guard against a size_t overflow:

  static size_t max_size_in_bytes()              { return max_size() * BytesPerWord; }

If need to care about overflow in MinTLABSizeConstraintFunc, don't we need to care about it in usages of max_size_in_bytes()?

src/hotspot/share/gc/z/zObjArrayAllocator.cpp line 63:

> 61:   assert(is_aligned(base_offset, HeapWordSize), "remaining array base must be 64 bit aligned");
> 62: 
> 63:   const size_t header = heap_word_size(base_offset);

`base_offset` has already been aligned by the code above, so we don't have to call heap_word_size here and can simply divide with HeapWordSize.

src/hotspot/share/opto/type.cpp line 5010:

> 5008:   if( _offset != 0 ) {
> 5009:     BasicType basic_elem_type = elem()->basic_type();
> 5010:     int header_size = arrayOopDesc::base_offset_in_bytes(basic_elem_type);

This function now calls base_offset_in_bytes twice. One to set header_size and the other to set the array_base. Maybe clean that up?

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

PR: https://git.openjdk.org/jdk/pull/11044