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

[Aleksey Shipilev]

On Wed, 5 Jul 2023 11:09:25 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 60 commits:
> 
>  - Merge branch 'master' into JDK-8139457
>  - Address @shipilev's comments
>  - Build fixes
>  - Merge branch 'master' into JDK-8139457
>  - Correctly handle oop array element aligment in 32bit builds; move method from Universe to Array
>  - Require uncompressed oops to be 8-byte-aligned
>  - Corresponding XGC fixes
>  - Merge branch 'master' into JDK-8139457
>  - Fix calls to removed instanceOopDesc::header_size()
>  - Add cast
>  - ... and 50 more: https://git.openjdk.org/jdk/compare/d6578bff...f1dbb859

More review comments (partial).

src/hotspot/share/c1/c1_LIRGenerator.cpp line 654:

> 652:     const int instance_size = align_object_size(klass->size_helper());
> 653:     __ allocate_object(dst, scratch1, scratch2, scratch3, scratch4,
> 654:                        oopDesc::header_size(),

Now that `oopDesc::header_size` is reverted, you can keep the argument list on the same line, to avoid the superfluous change.

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

> 321:     // ...and zap just allocated tlab.
> 322: #ifdef ASSERT
> 323:     Copy::fill_to_words(mem, allocation._allocated_tlab_size, badHeapWordVal);

Still not clear to me why we can zap the headers, given https://github.com/openjdk/jdk/commit/0917ad432eb3d01c104f03973c5c7ff52c6dfefe was needed for G1?

src/hotspot/share/oops/arrayOop.cpp line 1:

> 1: /*

Do you need this file now?

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

> 1: /*
> 2:  * Copyright (c) 1997, 2022, Oracle and/or its affiliates. All rights reserved.

Should be `2023`.

test/hotspot/gtest/oops/test_objArrayOop.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.

New file? If not, should be `2023`, at least.

test/hotspot/jtreg/gtest/ObjArrayTests.java line 2:

> 1: /*
> 2:  * Copyright (c) 2022 Amazon.com Inc. or its affiliates. All rights reserved.

Inconsistent header format: drop the year.

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

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

Inconsistent header format: drop the year.

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

PR Review: https://git.openjdk.org/jdk/pull/11044#pullrequestreview-1514308429
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252976727
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1253072169
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252975349
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252982236
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252981940
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252981132
PR Review Comment: https://git.openjdk.org/jdk/pull/11044#discussion_r1252981026