RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental) [v3]

Tue May 9 20:03:34 UTC 2023

On Mon, 8 May 2023 19:00:40 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>> 
>> Main changes:
>>  - Introduction of the (experimental) flag UseCompactObjectHeaders. All changes in this PR are protected by this flag.
>>  - The compressed Klass* can now be stored in the mark-word of objects. In order to be able to do this, we are building on #10907, #13582 and #13779 to protect the relevant (upper 32) bits of the mark-word. Significant parts of this PR deal with loading the compressed Klass* from the mark-word, and dealing with (monitor-)locked objects. When the object is monitor-locked, we load the displaced mark-word from the monitor, and load the compressed Klass* from there. This PR also changes some code paths (mostly in GCs) to be more careful when accessing Klass* (or mark-word or size) to be able to fetch it from the forwardee in case the object is forwarded, and/or reach through to the monitor when the object is locked by a monitor.
>>  - The identity hash-code is narrowed to 25 bits.
>>  - Instances can now have their base-offset (the offset where the field layouter starts to place fields) at offset 8 (instead of 12 or 16).
>>  - Arrays will can now store their length at offset 8. Due to alignment restrictions, array elements will still start at offset 16. #11044 will resolve that restriction and allow array elements to start at offset 12 (except for long, double and uncompressed oops, which are still required to start at an element-aligned offset).
>>  - CDS can now write and read archives with the compressed header. However, it is not possible to read an archive that has been written with an opposite setting of UseCompactObjectHeaders.
>> 
>> Testing:
>>  (+UseCompactObjectHeaders tests are run with the flag hard-patched into the build, to also catch @flagless tests, and to avoid mismatches with CDS - see above.)
>>  - [x] tier1 (x86_64)
>>  - [x] tier2 (x86_64)
>>  - [x] tier3 (x86_64)
>>  - [ ] tier4 (x86_64)
>>  - [x] tier1 (aarch64)
>>  - [x] tier2 (aarch64)
>>  - [x] tier3 (aarch64)
>>  - [ ] tier4 (aarch64)
>>  - [ ] tier1 (x86_64) +UseCompactObjectHeaders
>>  - [ ] tier2 (x86_64) +UseCompactObjectHeaders
>>  - [ ] tier3 (x86_64) +UseCompactObjectHeaders
>>  - [ ] tier4 (x86_64) +UseCompactObjectHeaders
>>  - [ ] tier1 (aarch64) +UseCompactObjectHeaders
>>  - [ ] tier2 (aarch64) +UseCompactObjectHeaders
>>  - [ ] tier3 (aarch64) +UseCompactObjectHeaders
>>  - [ ] tier4 (aarch64) +UseCompactObjectHeaders
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Allow to resolve mark with LW locking

Partial, cursory read...

src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 2355:

> 2353:       // Simple test for basic type arrays
> 2354:       if (UseCompressedClassPointers) {
> 2355:         __ load_nklass(tmp, src);

Is this entire thing a `cmp_klass`? x86 seems to do it with just `cmp_klass`.

src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp line 2360:

> 2358:       } else {
> 2359:         __ ldr(tmp, Address(src, oopDesc::klass_offset_in_bytes()));
> 2360:         __ ldr(rscratch1, Address(dst, oopDesc::klass_offset_in_bytes()));

Now that we inlined `src_klass_addr` and `dst_klass_addr` here, should we remove their definitions too? This would highlight if we have any paths that still use those addresses, perhaps by error?

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3581:

> 3579:         __ sub(r3, r3, BytesPerInt);
> 3580:         __ cbz(r3, initialize_header);
> 3581:       }

Things like these need to be protected by `UseCompactObjectHeaders`, to make it abundantly clear the legacy paths are unaffected.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3597:

> 3595:       __ mov(rscratch1, (intptr_t)markWord::prototype().value());
> 3596:       __ str(rscratch1, Address(r0, oopDesc::mark_offset_in_bytes()));
> 3597:       __ store_klass(r0, r4);      // store klass last

Where is `__ store_klass_gap(r0, zr)` from the original code?

src/hotspot/cpu/x86/c1_MacroAssembler_x86.cpp line 191:

> 189:       xorptr(t1, t1);
> 190:       movl(Address(obj, arrayOopDesc::length_offset_in_bytes() + sizeof(jint)), t1);
> 191:     }

The relevant block is missing at least in AArch64, should it be there too?

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

> 5155: 
> 5156: void MacroAssembler::store_klass(Register dst, Register src, Register tmp) {
> 5157:   assert(!UseCompactObjectHeaders, "not with compact headers");

The assert like that should be in all arches? Missing at least in AArch64.

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

> 5247: #ifdef _LP64
> 5248: void MacroAssembler::store_klass_gap(Register dst, Register src) {
> 5249:   assert(!UseCompactObjectHeaders, "Don't use with compact headers");

The assert like that should be in all arches? Missing at least in AArch64.

src/hotspot/cpu/x86/macroAssembler_x86.hpp line 377:

> 375: 
> 376:   // Compares the Klass pointer of two objects o1 and o2. Result is in the condition flags.
> 377:   // Uses t1 and t2 as temporary registers.

Suggestion:

  // Uses tmp1 and tmp2 as temporary registers.

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

PR Review: https://git.openjdk.org/jdk/pull/13844#pullrequestreview-1419326848
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189070425
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189047098
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189068421
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189069097
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189073559
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189075770
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189076056
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189076273