RFR: 8139457: Array bases are aligned at HeapWord granularity

Mon Dec 5 14:56:18 UTC 2022

On Tue, 8 Nov 2022 20:18:09 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)

Hi Roman,

nice work. Remarks inline.

Are these all adaptations needed? I wonder about things like SA that know data structure layout.

I ran the SAP nightlies with your patch and, as a test, UseCompressedClassPointers off. That generated a ton of noise unfortunately because many tests were unable to cope. A bunch of tests that created int arrays with Integer.MAX_VALUE-2 failed, but I soon discovered that they also fail in stock if you disable narrow class pointers.

But still, I think it would be useful to disable compressed class pointers (if not set at the command line, make the default false), if only temporarily, and then see which tests fail. I won't have any more time for this before the next year, unfortunately.

Cheers, Thomas

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

> 197: 
> 198:   // Zero first 4 bytes, if start offset is not word aligned.
> 199:   if (!is_aligned(hdr_size_in_bytes, BytesPerWord)) {

Add assertion for alignment to BytesPerInt?

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 4120:

> 4118: // Preserves the contents of address, destroys the contents length_in_bytes and temp.
> 4119: void MacroAssembler::zero_memory(Register address, Register length_in_bytes, int offset_in_bytes, Register temp) {
> 4120:   assert(address != length_in_bytes && address != temp && temp != length_in_bytes, "registers must be different");

While you are here, use "assert_different_registers"?

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 4121:

> 4119: void MacroAssembler::zero_memory(Register address, Register length_in_bytes, int offset_in_bytes, Register temp) {
> 4120:   assert(address != length_in_bytes && address != temp && temp != length_in_bytes, "registers must be different");
> 4121:   assert((offset_in_bytes & (BytesPerInt - 1)) == 0, "offset must be a multiple of BytesPerInt");

use is_aligned() (here and in other places)?

src/hotspot/cpu/x86/macroAssembler_x86.cpp line 4139:

> 4137:   jcc(Assembler::zero, done);
> 4138: #endif
> 4139: 

Just a style nit, my preference would have been to keep the assert section below at the beginning of the function since it guards the entry arguments. Change it to test for length_in_bytes alignment to BytesPerInt. Or, to either BytesPerInt or BytesPerWords, depending on how offset_in_bytes is aligned.

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

> 139: 
> 140:     const size_t max_size_bytes = align_down(SIZE_MAX - base_offset_in_bytes(type), MinObjAlignmentInBytes);
> 141:     const size_t max_elements_per_size_t = max_size_bytes / type2aelembytes(type);

Can we add an assert for max_size_bytes % type2aelembytes(type) == 0?

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

> 146:       // overflowing an int when we add the header. See CRs 4718400 and 7110613.
> 147:       int header_size_words = align_up(base_offset_in_bytes(type), HeapWordSize) / HeapWordSize;
> 148:       return align_down(max_jint - header_size_words, MinObjAlignment);

I somehow was under the impression that for smaller types we should now have a larger max, since we can fit more elements.

src/hotspot/share/oops/objArrayOop.hpp line 84:

> 82:     size_t osz = align_object_size(size_words);
> 83:     assert(osz < max_jint, "no overflow");
> 84:     return osz;

Here, and for some of the other basic functions, some gtests would be nice. Mainly to test outliers like -UseCompressedClassPointers +UseCompressedOops, and with different values for ObjAlignmentInBytes. Can be as simple as this:

TEST_VM(objArrayOopDesc, osize) {
  static const struct {
    int objal; bool ccp; bool coops; int result;
  } x[] = {
//    ObjAligInB, UseCCP, UseCoops, object size in heap words
#ifdef _LP64
    { 8,          false,  false,    4 }, // 20 byte header, 8 byte oops
    { 8,          false,  true,     3 }, // 20 byte header, 4 byte oops
    { 8,          true,   false,    3 }, // 16 byte header, 8 byte oops
    { 8,          true,   true,     3 },  // 16 byte header, 4 byte oops
    { 256, ..... 
    // etc..
#else
    { 8,          false,  false,    4 }, // 12 byte header, 4 byte oops, wordsize 4
#endif
    { -1, false, false, -1 }
  };
  for (int i = 0; x[i].result != -1; i++) {
    if (x[i].objal == (int)ObjectAlignmentInBytes && x[i].ccp == UseCompressedClassPointers && x[i].coops == UseCompressedOops) {
      EXPECT_EQ(objArrayOopDesc::object_size(1), (size_t)x[i].result);
    }
  }
}

and then execute them via the gtest jtreg runner with different settings. For examples, see e.g. https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/gtest/NMTGtests.java 

64bit, UseCCP, UseCoops

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

> 35:  * @modules java.base/jdk.internal.misc
> 36:  * @run main/othervm -XX:+UseCompressedClassPointers ArrayBaseOffsets
> 37:  */

I'd run all permutations of +-UseCCP +-UseCoops

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

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