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

[Aleksey Shipilev]

On Wed, 1 Feb 2023 17:09:21 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:
> 
>   Remove stale method

Still have a few questions.

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

> 255:   const size_t elements_per_word = HeapWordSize / sizeof(jint);
> 256:   int base_offset_in_ints = arrayOopDesc::base_offset_in_ints(T_INT);
> 257:   _filler_array_max_size = align_object_size((base_offset_in_ints + max_len) / elements_per_word);

Isn't this expression susceptible to overflow, like the removed comment in `CollectedHeap::max_tlab_size` (below) states? I.e. max_len is probably very close to SIZE_MAX on 32-bit platforms, and adding the base offset gets dangerously close there. Not to mention the positive side of signed `int` domain is lower than SIZE_MAX to beging with? I think you need to keep doing the division `max_len / elements_per_word` first.

src/hotspot/share/oops/arrayOop.hpp line 136:

> 134:   }
> 135: 
> 136:   // Return the maximum length (num elements) of an array of BasicType.  The length can passed

"(num elements)" addition here is unnecessary, I think.

test/hotspot/gtest/oops/test_objArrayOop.cpp line 46:

> 44:     { 8,          false,  false,    4 }, // 12 byte header, 4 byte oops, wordsize 4
> 45: #endif
> 46:     { -1, false, false, -1 }

Indentation compared to similar cases above.

test/hotspot/jtreg/runtime/FieldLayout/ArrayBaseOffsets.java line 110:

> 108:         Asserts.assertEquals(unsafe.arrayBaseOffset(float[].class),   intOffset,  "Misplaced float   array base");
> 109:         Asserts.assertEquals(unsafe.arrayBaseOffset(double[].class),  longOffset, "Misplaced double  array base");
> 110:         int expectedObjArrayOffset = (COOP || !Platform.is64bit()) ? intOffset : longOffset;

`|| !Platform.is64Bit()` looks redundant, since `COOP` is guaranteed to be false on 32-bit?

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

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