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

[Aleksey Shipilev]

On Tue, 24 Jan 2023 14:48:26 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:
> 
>   Move filler_array_min_size() into inline header file

Cursory review follows.

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

> 415: void CollectedHeap::zap_filler_array_with(HeapWord* start, size_t words, juint value) {
> 416:   int payload_start = arrayOopDesc::base_offset_in_bytes(T_INT);
> 417:   if (!is_aligned(payload_start, BytesPerWord)) {

So, this should also be `..., HeapWordSize` to match the post-correction assert, or do I miss some other intent?

src/hotspot/share/gc/shared/collectedHeap.inline.hpp line 50:

> 48: 
> 49: inline size_t CollectedHeap::filler_array_min_size() {
> 50:   size_t aligned_header_size_words = heap_word_size(arrayOopDesc::base_offset_in_bytes(T_INT));

Why this is called `aligned_*`? Looks like it becomes aligned only after the `align_object_size`?

src/hotspot/share/oops/arrayKlass.cpp line 128:

> 126: 
> 127: objArrayOop ArrayKlass::allocate_arrayArray(int n, int length, TRAPS) {
> 128:   check_array_allocation_length(length, arrayOopDesc::max_array_length(element_type()), CHECK_NULL);

Wait, why? This method allocates the array of the "this"-typed arrays, I think. I.e. if `this` is `int[]`, then `element_type()` is `int`, and then `allocate_arrayArray(y, len)` allocates `int[length][]`, and we still need to check `length` against `max_array_length(T_ARRAY)`? Which is also why we do `objArrayOopDesc::object_size` at the very next line.

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

> 93:     return (int)(element_type_should_be_aligned(type)
> 94:                  ? align_up(hs, BytesPerLong)
> 95:                  : hs);

Style: you can write this in one line, I think.

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

> 73:     // This returns the object size in HeapWords.
> 74:     size_t asz = (size_t)length * heapOopSize;
> 75:     size_t size_words = align_up(base_offset_in_bytes() + asz, HeapWordSize) / HeapWordSize;

Is this `heap_word_size`?

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

> 310:     BasicType elem_type = TypeArrayKlass::cast(array_type)->element_type();
> 311:     size_t hs_bytes = arrayOopDesc::base_offset_in_bytes(elem_type);
> 312:     assert(is_aligned(hs_bytes, BytesPerInt), "must be 4 byte aligned");

This assert is misplaced, should be in the `if` block below? Otherwise checking for HeapWordSize alignment after assert-checking for BytesPerInt seems rather odd: the check is guaranteed to pass if assert does not fire.

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

> 98:       EXPECT_EQ(arrayOopDesc::base_offset_in_bytes(T_OBJECT), 16);
> 99:       EXPECT_EQ(arrayOopDesc::base_offset_in_bytes(T_ARRAY),  16);
> 100:     }

These branches are the same; merge them? It makes sense that `T_OBJECT`/`T_ARRAY` base offset does not depend on `UseCompressedOops` under `+UseCompressedClassPointers`.

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

> 1: /*
> 2:  * Copyright (C2 2022, Amazon.com Inc. or its affiliates. All Rights Reserved.

Typo: `(C2`

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

> 23: 
> 24: /*
> 25:  * @test id=with-coop-no-ccp

`with-coops-no-ccp` to match the ID style in this and other tests?

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

> 38:  */
> 39: /*
> 40:  * @test id=no-coop-no-ccp

`no-coops-no-ccp` to match the ID style in this and other tests?

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

> 96:         Asserts.assertEquals(unsafe.arrayBaseOffset(float[].class),   intOffset,  "Misplaced float   array base");
> 97:         Asserts.assertEquals(unsafe.arrayBaseOffset(double[].class),  longOffset, "Misplaced double  array base");
> 98:         boolean narrowOops = System.getProperty("java.vm.compressedOopsMode") != null ||

It feels a bit weird to sense `-XX:-UseCompressedClassPointers` (and thus rely on test command line), but poll the system property here. Suggestion: do two `static final`-s, `COOP` and `CCP`, and then define them in `static` initializer.

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

> 98:         boolean narrowOops = System.getProperty("java.vm.compressedOopsMode") != null ||
> 99:                              !Platform.is64bit();
> 100:         int expected_objary_offset = narrowOops ? intOffset : longOffset;

Not a Java style for identifier, should be `expectedObjArrOffset`.

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

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