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

[Axel Boldt-Christmas]

On Wed, 23 Aug 2023 14:16:16 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:
> 
>   Fix wrong alignment

Only some inconsistencies I found with regards to parameter names and comments. 
Other than that it looks correct. As I noted in an earlier comment the ZGC changes is something we can cleanup in a followup PR. 

As for the C1 changes it would be nice to refactor it to not clear the parts that are part of the body in the header initialisation. But personally I do not believe it has to stall this PR. But I'll defer that. No strong opinion.

As for the nomenclature surrounding the header, some effort into making it more clear and consistent would be appreciated. 

This is just a tangential comment:
Personally I would prefer to have the `<...>_in_bytes` refer to offsets or sizes in bytes and `<...>` refer to them in the smallest word size which contain the exact property. Also have a naming distinction between the `header` and the `array_header`. Mostly because every object has a header but only arrays have an array_header. The `header_size` (in words) and `array_header_size` may be, but `header_size_in_bytes`  and `array_header_size_in_bytes` are different.

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

> 57:   // aligned 0 mod 8.  The typeArrayOop itself must be aligned at least this
> 58:   // strongly.
> 59:   static bool element_type_should_be_aligned(BasicType type) {

The comment above makes it sound like this is only valid for `typeArrayOop`. Should be updated to reflect the change.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Array.java line 63:

> 61:   // aligned 0 mod 8.  The typeArrayOop itself must be aligned at least this
> 62:   // strongly.
> 63:   private static boolean elementTypeShouldBeAligned(BasicType type) {

Same comment as `arrayOopDesc::element_type_should_be_aligned`

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

PR Review: https://git.openjdk.org/jdk/pull/11044#pullrequestreview-1591458933
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1303370974
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1303392574