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

[Roman Kennke]

On Fri, 18 Aug 2023 07:44:41 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> 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.

Thanks, Stefan for checking this. Yes, arrayOop::header_size() is confusing. The only purpose of this method is to help a few code paths which need the header_size_in_bytes() to be aligned-up and converted to word-size. Not quite sure how to deal with it. Move it out to some different place? Name it differently?

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

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