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

Fri May 12 16:19:59 UTC 2023

On Fri, 12 May 2023 12:10:16 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:
> 
>   Some hashcode improvements (mostly SA)

I don't have any comments on the compiler code or gc code, but some other comments and questions.  Some of the LP64 preprocessor conditionals are inconsistent in the assembly code.

src/hotspot/share/gc/parallel/psPromotionManager.cpp line 293:

> 291: 
> 292:   oop old = task.to_source_array();
> 293:   assert(old->forward_safe_klass()->is_objArray_klass(), "invariant");

Why sometimes forward_safe_klass()?  Shouldn't all calls to klass() be "forward_safe"?  How do you know where to put this version of the klass() call?

src/hotspot/share/memory/universe.cpp line 325:

> 323:     assert(oopDesc::klass_offset_in_bytes() < static_cast<intptr_t>(os::vm_page_size()),
> 324:            "Klass offset is expected to be less than the page size");
> 325:   }

This is where you should have else mark_offset_in_bytes() < page size or maybe it should be changed to the needs_explicit_null_check_code().

src/hotspot/share/oops/klass.cpp line 207:

> 205:   return prototype;
> 206: }
> 207: 

This seems like a useful change without UseCompactObjectHeaders as an enhancement and to remove some conditional code.  Since we have storage in Klass for it anyway.

src/hotspot/share/oops/objArrayKlass.cpp line 160:

> 158: size_t ObjArrayKlass::oop_size(oop obj) const {
> 159:   // In this assert, we cannot safely access the Klass* with compact headers.
> 160:   assert(UseCompactObjectHeaders || obj->is_objArray(), "must be object array");

Isn't there code that checks oop->is_objArray() before calling this?  Would it return true when it's not an objArray?

src/hotspot/share/oops/oop.inline.hpp line 126:

> 124: 
> 125: Klass* oopDesc::klass_or_null() const {
> 126: #ifdef _LP64

I don't like all these #ifdef _LP64 here.  Maybe markWord.inline.hpp can be refactored to not require callers to have this conditional inclusion.

src/hotspot/share/runtime/arguments.cpp line 3136:

> 3134:   if (UseCompactObjectHeaders && !UseCompressedClassPointers) {
> 3135:     FLAG_SET_DEFAULT(UseCompressedClassPointers, true);
> 3136:   }

Make this a function like set_compact_object_headers_flags(), that checks for FLAG_IS_CMDLINE for the related options and give a warning for them too, and add a test.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13844#pullrequestreview-1423518398
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192518760
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192536609
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192539910
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192548449
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192552486
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1192558364