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

[Stefan Karlsson]

On Thu, 17 Aug 2023 19:07:36 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 two additional commits since the last revision:
> 
>  - Move alignment-gap-clearing into allocate_array() (aarch64)
>  - Move header_size_in_bytes closer to length_offset_in_bytes

There is something odd going on with the `arrayOopDesc::header_size()` implementation. In some configurations the array length is within the header size, but that's not always the case.

This picture shows "header_size_in_bytes()" for different 64-bits configurations:

 ====================================== Array ========================================
 ===================== Full Klass ====================         === Compressed Klass ==
  Aligned Field                   Unaligned Field                Auto Aligned Field     
+-----------------------+     +-----------------------+       +-----------------------+ 
| Mark 0                |     | Mark 0                |       | Mark 0                | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Mark 1                |     | Mark 1                |       | Mark 1                | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Klass 0               |     | Klass 0               |       | Compressed Klass      | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Klass 1               |     | Klass 1               |       | Length                | 
+-----------------------+     +-----------------------+    +- +-----------------------+ 
| Length                |     | Length                |    |  | Field 0               |  
+-----------------------+ -+  +-----------------------+ -+ |  +-----------------------+  
| Gap                   |  |  | Field 0               |  | |  | Field 1               |  
+-----------------------+  |  +-----------------------+  | |  +-----------------------+  
| Field 0               |  |  | Field 1               |  | |  
+-----------------------+  |  +-----------------------+  | |  
| Field 1               |  |  | Padding               |  | |  
+-----------------------+  |  +-----------------------+  | |   
                           |                             | |  
                           +-----------------------------+-+
                           |
			   +------- new arrayOopDesc::header_size_in_bytes()

This looks reasonable. If I draw the same picture for `header_size()` I get this:

 ====================================== Array ========================================
 ===================== Full Klass ====================         === Compressed Klass ==
  Aligned Field                  Unaligned Field                 Auto Aligned Field     
+-----------------------+     +-----------------------+       +-----------------------+ 
| Mark 0                |     | Mark 0                |       | Mark 0                | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Mark 1                |     | Mark 1                |       | Mark 1                | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Klass 0               |     | Klass 0               |       | Compressed Klass      | 
+-----------------------+     +-----------------------+       +-----------------------+ 
| Klass 1               |     | Klass 1               |       | Length                | 
+-----------------------+ -+  +-----------------------+    +- +-----------------------+ 
| Length                |  |  | Length                |    |  | Field 0               |  
+-----------------------+  |  +-----------------------+    |  +-----------------------+  
| Gap                   |  |  | Field 0               |    |  | Field 1               |  
+-----------------------+  |  +-----------------------+ -+ |  +-----------------------+  
| Field 0               |  |  | Field 1               |  | |  
+-----------------------+  |  +-----------------------+  | |  
| Field 1               |  |  | Padding               |  | |  
+-----------------------+  |  +-----------------------+  | |   
                           |                             | |  
                           +-----------------------------+-+
			   |
			   +------- new arrayOopDesc::header_size()


I've verified that this is the case by running the following gtest:

TEST_VM(arrayOopDesc, header_size_in_bytes) {
#ifdef _LP64
  {
    FlagSetting fs(UseCompressedClassPointers, false);
    // Array length starts at 16
    EXPECT_EQ(arrayOopDesc::length_offset_in_bytes(), 16);

    // Header size in bytes includes the length.
    EXPECT_EQ(arrayOopDesc::header_size_in_bytes(), 20);

    // arrayOopDesc::header_size implies that one of the elements are included in the header
    EXPECT_EQ(arrayOopDesc::header_size(T_BOOLEAN) * HeapWordSize, 24);

    // Without compressed oops not even the length is included in the header
    FlagSetting fs2(UseCompressedOops, false);
    EXPECT_EQ(arrayOopDesc::header_size(T_OBJECT) * HeapWordSize,  16);

    // With compressed oops the length and and one of the oops are included in the header
    FlagSetting fs3(UseCompressedOops, true);
    EXPECT_EQ(arrayOopDesc::header_size(T_OBJECT) * HeapWordSize,  24);
  }
#endif
}

(Note: I had to turn off the assert in `header_size_in_bytes()` to get the test to work)

The problem is that `header_size_in_bytes` used to return a HeapWordSize-aligned value, which allowed for a non-lossy division in the line `align_object_offset(typesize_in_bytes/HeapWordSize)`.

I think this should be fixed and I think it would make sense to add complete tests for arrayOopDesc::heap_size(), similar to what you did for base_offset_in_bytes.

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

PR Comment: https://git.openjdk.org/jdk/pull/11044#issuecomment-1683504300