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

[Stefan Karlsson]

On Fri, 13 Jan 2023 19:04:59 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 with a new target base due to a merge or a rebase. The pull request now contains 25 commits:
> 
>  - Merge branch 'master' into JDK-8139457
>  - Fix gtest for correct base offsets in 32bit builds
>  - Fix cast warning
>  - Revert relaxation of array length
>  - Add guards to ArrayBaseOffsets test to allow running with -UseCompressedClassPointers
>  - Fix another cast warning
>  - Clean cast warning from size_t to int
>  - Clear leading 32bits in Z array allocator
>  - SA adjustments
>  - Test for 32bit build
>  - ... and 15 more: https://git.openjdk.org/jdk/compare/500b45e1...c278a53b

I tried to read the code very carefully, but it's not entirely easy to review a patch like this. I think I found two bugs, and a number of things that I think could help the readability of the code.

I didn't look to closely at the tests, but I think I'd like to see a test that checks the boundary conditions of the max array calculations.

I'd suggest that more Reviewers take a close look at the code.

src/hotspot/cpu/ppc/c1_MacroAssembler_ppc.cpp line 376:

> 374:     addi(base, base, BytesPerInt);
> 375:     // Note: initialize_body will align index down, no need to correct it here.
> 376:   }

What do `dword` refer to here? Do you mean `word`?

For if statements, I think it's good to consider if the comment describes the conditional, or what happens if the conditional is true. The comment above describes what happens if it is true, so it would be better to move it to after Line 371. Alternatively, rewrite the comment to say "Check if elements are not word aligned", and move the "Zero out leading word". Or change it two the riscv comment:
`// Zero first 4 bytes, if start offset is not word aligned.`

src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 276:

> 274:   Register t1,                         // temp register
> 275:   Register t2,                         // temp register
> 276:   int      base_offset_in_bytes,       // offset of array elements in bytes

This comment is not consistent with the PPC comment:
`int base_offset_in_bytes, // elements offset in bytes`

src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 313:

> 311:   Register object_fields = t1;
> 312:   Register Rzero = Z_R1_scratch;
> 313:   z_aghi(arr_size, -(base_offset_in_bytes));

Maybe remove the brackets?
`z_aghi(arr_size, -(base_offset_in_bytes));`

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

> 4134:   assert(is_aligned(offset_in_bytes, BytesPerInt), "offset must be a multiple of BytesPerInt");
> 4135: 
> 4136:   // note: for the remaining code to work, index must be a multiple of BytesPerInt

Maybe move the comment to above the assert that checks the invariant described by the comment?

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

> 4152:   // Emit single 32bit store to clear leading bytes, if necessary.
> 4153:   xorptr(temp, temp);    // use _zero reg to clear memory (shorter code)
> 4154: #ifdef _LP64

Since temp is used outside of the section L4152-4164, I think it would be appropriate to hoist it above the comment on 4152.

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

> 256:   int header_size_in_bytes = arrayOopDesc::base_offset_in_bytes(T_INT);
> 257:   assert(is_aligned(header_size_in_bytes, BytesPerInt), "must be aligned to int");
> 258:   int header_size_in_ints = header_size_in_bytes / BytesPerInt;

I wonder if it would make sense to create an arrayOopDesc::base_offset_in_ints(T_INT)?

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

> 425:               ((juint) max_jint / (size_t) HeapWordSize);
> 426:   return align_down(max_int_size, MinObjAlignment);
> 427: }

Why isn't this using `_filler_array_max_size` instead of calculating its own value? Are these two values different, and if so, why?

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

> 429: size_t CollectedHeap::filler_array_min_size() {
> 430:   size_t aligned_header_size_words = heap_word_size(arrayOopDesc::base_offset_in_bytes(T_INT));
> 431:   return align_object_size(aligned_header_size_words); // align to MinObjAlignment

src/hotspot/share/gc/parallel/mutableNUMASpace.cpp makes the same calculation. There might be an opportunity to extract these two calculations into a helper function.

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

> 442:   payload_start = payload_start / HeapWordSize;
> 443:   Copy::fill_to_words(start + payload_start,
> 444:                       words - payload_start, value);

This mixes bytes and words. I don't think this is correct.

src/hotspot/share/gc/shared/memAllocator.cpp line 427:

> 425:   }
> 426:   ArrayKlass* array_klass = ArrayKlass::cast(_klass);
> 427:   const size_t hs = heap_word_size(arrayOopDesc::base_offset_in_bytes(array_klass->element_type()));

I don't think this makes sense. If base_offset_in_bytes return something that isn't word aligned, then `heap_word_size` will truncate the value and `hs` will less than the actual header size. This then snowballs back to the caller of `obj_memory_range`. Maybe we need change obj_memory_range to work with bytes instead of words?

src/hotspot/share/gc/z/zObjArrayAllocator.cpp line 55:

> 53:   int base_offset = arrayOopDesc::base_offset_in_bytes(element_type);
> 54: 
> 55:   // Clear leading 32 bit, if necessary.

32 bit => 32 bits?

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

> 52:   // sizeof(arrayOopDesc) which should not appear in the code.
> 53:   static int header_size_in_bytes() {
> 54:     size_t hs = length_offset_in_bytes() + sizeof(int);

The comment above states that this returns the "aligned" value. It doesn't, so I think the comment needs to be updated.

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

> 94:     return (int)(element_type_should_be_aligned(type)
> 95:                  ? align_up(typesize_in_bytes, BytesPerLong)
> 96:                  : typesize_in_bytes);

I think that the name typesize is confusing here. Maybe just use a short name for this very short function. Say, `value` or `hs`?

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

> 139:     assert(type2aelembytes(type) != 0, "wrong type");
> 140: 
> 141:     int elem_size = type2aelembytes(type);

The rest of the nearby locals seems to be declared const, so maybe follow that style here?

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

> 141:     int elem_size = type2aelembytes(type);
> 142:     const size_t max_size_bytes = align_down(SIZE_MAX - base_offset_in_bytes(type), MinObjAlignmentInBytes);
> 143:     assert(max_size_bytes % elem_size == 0, "max_size_bytes should be aligned to element size");

Use is_aligned?

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

> 149:       // overflowing an int when we add the header. See CRs 4718400 and 7110613.
> 150:       size_t header_size_words = heap_word_size(base_offset_in_bytes(type));
> 151:       return align_down(max_jint - static_cast<int>(header_size_words), MinObjAlignment);

Can't immediately tell if this is correct, given that `heap_word_size` truncates the header. (Haven't checked yet if you have written unittests for this).

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

> 53: private:
> 54:   // Give size of objArrayOop in bytes minus the header
> 55:   static size_t array_size_in_bytes(int length) {

This is such a misleading name. I wouldn't mind getting rid of this function by inlining it into the its single call site.

src/hotspot/share/opto/runtime.cpp line 316:

> 314:     HeapWord* obj = cast_from_oop<HeapWord*>(result);
> 315:     if (aligned_hs_bytes > hs_bytes) {
> 316:       Copy::zero_to_bytes(obj + hs_bytes, aligned_hs_bytes - hs_bytes);

I think this is wrong. `obj` is a `HeapWord*` and hs_bytes is in bytes. You shouldn't be adding to a pointer with a variable holding bytes.

test/hotspot/gtest/oops/test_arrayOop.cpp line 83:

> 81: // T_VOID and T_ADDRESS are not supported by max_array_length()
> 82: 
> 83: 

Nit: spurious newline

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

Changes requested by stefank (Reviewer).

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