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

Fri May 12 16:20:05 UTC 2023

On Thu, 11 May 2023 19:25:46 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 two additional commits since the last revision:
> 
>  - Fix some uses of klass_offset_in_bytes()
>  - Fix args checking

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

> 328:   } else {
> 329:     assert(!MacroAssembler::needs_explicit_null_check(oopDesc::klass_offset_in_bytes()), "must add explicit null check");
> 330:   }

We put this check in Universe::genesis() so it's not needed here for one less conditional.  Maybe that check should be this one instead of what we have there.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 4322:

> 4320:   assert(UseCompactObjectHeaders, "expects UseCompactObjectHeaders");
> 4321: 
> 4322:   if (!UseCompactObjectHeaders) {

I'm confused, why is this conditional here if you asserted it before? I can't imagine this being an untested code path and you need this for safety.  If so, this doesn't take CompressedKlassPointers into account.  I think it would be better to remove it.  If I'm reading this right.  Maybe change this assert to a guarantee for testing if you think this is likely.

I see why this is.  This is inconsistent with x86.  You should fix this to match x86 and make it load_narrow_klass().

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

> 165: void C1_MacroAssembler::initialize_header(Register obj, Register klass, Register len, Register t1, Register t2) {
> 166:   assert_different_registers(obj, klass, len, t1, t2);
> 167:   if (UseCompactObjectHeaders) {

Shouldn't this be in _LP64 too like the code just above?

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

> 184:   if (len->is_valid()) {
> 185:     movl(Address(obj, arrayOopDesc::length_offset_in_bytes()), len);
> 186:     if (UseCompactObjectHeaders) {

This should also be in _LP64 and not have && !UseCompactObjectHeaders. You should restrict this to LP64 in this change.

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

> 321:   } else {
> 322:     assert(!MacroAssembler::needs_explicit_null_check(oopDesc::klass_offset_in_bytes()), "must add explicit null check");
> 323:   }

I think this should be removed in favor of the test in Universe::genesis.

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

> 365:   // oop manipulations
> 366: #ifdef _LP64
> 367:   void load_nklass(Register dst, Register src);

Should this be private?  Is it only called by load_klass ?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191740593
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191744083
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191757984
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191758786
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191759600
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191762136