RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental) [v3]
Aleksey Shipilev
shade at openjdk.org
Wed May 10 11:28:53 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
Another round!
src/hotspot/cpu/x86/sharedRuntime_x86.cpp line 75:
> 73: uintptr_t mask_in_place = UseCompactObjectHeaders ? markWord::hash_mask_in_place_compact : markWord::hash_mask_in_place;
> 74: __ shrptr(result, shift);
> 75: __ andptr(result, mask);
Please conditionalize it with `if (UseCompactObjectHeaders)` to make the distinction between the paths cleaner.
src/hotspot/cpu/x86/templateTable_x86.cpp line 4034:
> 4032: // The object is initialized before the header. If the object size is
> 4033: // zero, go directly to the header initialization.
> 4034: int header_size = align_up(oopDesc::base_offset_in_bytes(), BytesPerLong);
Please conditionalize with `if (UseCompactObjectHeaders)`.
src/hotspot/cpu/x86/templateTable_x86.cpp line 4068:
> 4066: __ pop(rcx); // get saved klass back in the register.
> 4067: __ movptr(rbx, Address(rcx, Klass::prototype_header_offset()));
> 4068: __ movptr(Address(rax, oopDesc::mark_offset_in_bytes ()), rbx);
Suggestion:
__ movptr(Address(rax, oopDesc::mark_offset_in_bytes()), rbx);
src/hotspot/cpu/x86/x86_64.ad line 5346:
> 5344: __ jcc(Assembler::notZero, stub->entry());
> 5345: __ bind(stub->continuation());
> 5346: __ shrq(dst, markWord::klass_shift);
Any reason not to do this thing in `MacroAssembler`?
src/hotspot/share/gc/parallel/psOldGen.cpp line 398:
> 396:
> 397: virtual void do_object(oop obj) {
> 398: HeapWord* test_addr = cast_from_oop<HeapWord*>(obj);
I thought this `+1` is specifically to test that `object_start` is able to find the object header when given the interior pointer. See the `guarantee`-s in the next lines.
src/hotspot/share/gc/parallel/psPromotionManager.inline.hpp line 250:
> 248: Copy::aligned_disjoint_words(cast_from_oop<HeapWord*>(o), cast_from_oop<HeapWord*>(new_obj), new_obj_size);
> 249:
> 250: if (!new_obj->mark().is_marked()) {
Oh, this is the same as for Shenandoah. See the comment there: probably want to condition this on `UseCompactObjectHeaders`.
src/hotspot/share/gc/serial/markSweep.cpp line 175:
> 173: }
> 174:
> 175: ContinuationGCSupport::transform_stack_chunk(obj);
Add a comment, something like: "// Do the transform while we still have the header intact, which might include important class information".
src/hotspot/share/gc/shared/collectedHeap.cpp line 232:
> 230: // With compact headers, we can't safely access the class, due
> 231: // to possibly forwarded objects.
> 232: if (!UseCompactObjectHeaders && is_in(object->klass_raw())) {
Looks good, but what this even supposed to check? `object` is not `oop` if its klass field points into Java heap? Huh? Was it some CMS shenanigan that stores something in klass word? Or is it just a glorified null check? I'll follow up on that separately.
src/hotspot/share/gc/shared/collectedHeap.hpp line 312:
> 310:
> 311: virtual void fill_with_dummy_object(HeapWord* start, HeapWord* end, bool zap);
> 312: static size_t min_dummy_object_size() {
Why this change?
src/hotspot/share/gc/shared/gc_globals.hpp line 692:
> 690: constraint(GCCardSizeInBytesConstraintFunc,AtParse) \
> 691: \
> 692: product(bool, UseAltGCForwarding, false, \
Should it be `EXPERIMENTAL`?
src/hotspot/share/gc/shared/memAllocator.cpp line 414:
> 412: // concurrent collectors.
> 413: if (UseCompactObjectHeaders) {
> 414: oopDesc::release_set_mark(mem, _klass->prototype_header());
In other cases, we do `markWord::prototype().set_narrow_klass(nk)` -- it looks safer, as we get the `markWord`-s prototype, and amend it. `_klass->prototype_header` can be removed, I think.
src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 326:
> 324: oop copy_val = cast_to_oop(copy);
> 325: if (!copy_val->mark().is_marked()) {
> 326: // If we copied a mark-word that indicates 'forwarded' state, then
Ouch. This is only the problem with `UseCompactObjectHeaders`, right? Can additionally conditionalize on that, so that legacy code path stays the same.
src/hotspot/share/gc/shenandoah/shenandoahVerifier.cpp line 309:
> 307: _loc = obj;
> 308: Klass* klass = obj->forward_safe_klass();
> 309: obj->oop_iterate_backwards(this, klass);
Why `backwards`?
src/hotspot/share/gc/z/zRelocate.cpp line 339:
> 337: if (SuspendibleThreadSet::should_yield()) {
> 338: SuspendibleThreadSet::yield();
> 339: }
These should be upstreamed separately, like we did with Shenandoah STS additions?
src/hotspot/share/oops/klass.hpp line 675:
> 673: void set_is_cloneable();
> 674:
> 675: markWord prototype_header() const {
Suggestion:
markWord prototype_header() const {
src/hotspot/share/oops/markWord.hpp line 238:
> 236: uintptr_t mask = UseCompactObjectHeaders ? hash_mask_compact : hash_mask;
> 237: int shift = UseCompactObjectHeaders ? hash_shift_compact : hash_shift;
> 238: uintptr_t tmp = value() & ~mask_in_place;
No parenthesis left behind
Suggestion:
uintptr_t tmp = value() & (~mask_in_place);
src/hotspot/share/oops/oop.cpp line 176:
> 174: return obj->klass();
> 175: } else
> 176: #endif
Here and everywhere else: if we add `else` under define, we need to wrap the non-ifdefed code with `{}`.
#ifdef _LP64
if (...) {
...
} else
#endif
{
...
}
src/hotspot/share/oops/oop.hpp line 124:
> 122: inline size_t size_given_klass(Klass* klass);
> 123:
> 124: // The following set of methods is used to access the mark-word and related
So, these are done to avoid introducing branches on the paths where objects are definitely _not_ forwarded? Are there fewer places than where we expect forwardings? Maybe the better way would be to make all methods handle the occasional forwarding, and then provide the methods that provide the _fast-path_, like `fast_mark`, `fast_class`, etc?
src/hotspot/share/oops/oop.hpp line 130:
> 128: // those methods can not deal with the sliding-forwarding that is used
> 129: // in Serial, G1 and Shenandoah full-GCs.
> 130: inline markWord forward_safe_mark() const;
`forward_safe_mark` seems to be only used in `forward_safe_klass`, can be inlined/simplified there?
src/hotspot/share/oops/oop.hpp line 346:
> 344: // load the narrowKlass from the header.
> 345: STATIC_ASSERT(markWord::klass_shift % 8 == 0);
> 346: return mark_offset_in_bytes() + markWord::klass_shift / 8;
There is a convenient `BitsPerByte` constant for this -- makes it clear we are converting bits to bytes.
src/hotspot/share/oops/oop.hpp line 362:
> 360: // With compact headers, the Klass* field is not used for the Klass*
> 361: // and is used for the object fields instead.
> 362: assert(sizeof(markWord) == 8, "sanity");
`STATIC_ASSERT`?
src/hotspot/share/oops/oop.inline.hpp line 106:
> 104: #ifdef _LP64
> 105: if (UseCompactObjectHeaders) {
> 106: assert(UseCompressedClassPointers, "expect compressed klass pointers");
Here and in other places in this file, it seems redundant to check `UseCompressedClassPointers`, as argument checking code makes sure we are in the right mode? We don't seem to check it consistently anyway.
src/hotspot/share/oops/oop.inline.hpp line 261:
> 259:
> 260: markWord oopDesc::forward_safe_mark() const {
> 261: markWord mrk = mark();
Convention: `mrk` -> `m`.
src/hotspot/share/oops/oop.inline.hpp line 491:
> 489: template <typename OopClosureType>
> 490: void oopDesc::oop_iterate_backwards(OopClosureType* cl, Klass* k) {
> 491: // We cannot safely access the Klass* with compact headers here.
This comment is only about the assert itself, right? If so, then:
// In this assert, ...
src/hotspot/share/oops/typeArrayKlass.cpp line 231:
> 229:
> 230: size_t TypeArrayKlass::oop_size(oop obj) const {
> 231: assert(obj->is_typeArray(),"must be a type array");
So, these checks are removed because they reach for class, which we cannot use with `UseCompactObjectHeaders`? Better to disable the assert at this time, I think, instead of removing it.
src/hotspot/share/oops/typeArrayKlass.inline.hpp line 38:
> 36:
> 37: inline void TypeArrayKlass::oop_oop_iterate_impl(oop obj, OopIterateClosure* closure) {
> 38: // We cannot safely access the Klass* with compact headers.
Same thing about assert? "In this assert, ..."
src/hotspot/share/opto/callnode.cpp line 1579:
> 1577: Node* klass_node = in(AllocateNode::KlassNode);
> 1578: Node* proto_adr = phase->transform(new AddPNode(klass_node, klass_node, phase->MakeConX(in_bytes(Klass::prototype_header_offset()))));
> 1579: mark_node = LoadNode::make(*phase, control, mem, proto_adr, TypeRawPtr::BOTTOM, TypeX_X, TypeX_X->basic_type(), MemNode::unordered);
Note to self: This load is probably foldable if we know the klass is constant -- which it almost always is on allocation paths. I'll check and fix if it is not.
src/hotspot/share/runtime/arguments.cpp line 3120:
> 3118:
> 3119: #ifdef _LP64
> 3120: if (!FLAG_IS_DEFAULT(UseCompactObjectHeaders)) {
Just `if (UseCompactObjectHeaders)`, or do I miss something?
src/hotspot/share/runtime/arguments.cpp line 3128:
> 3126: warning("Compact object headers require compressed class pointers. Disabling compact object headers.");
> 3127: FLAG_SET_DEFAULT(UseCompactObjectHeaders, false);
> 3128: }
I think you need to print the warning when user overrides it, but disable it _even if user did not overridden it_ (e.g. there is a flag selection bug somewhere in platform-specific VM code).
Suggestion:
if (UseCompactObjectHeaders && !UseCompressedClassPointers) {
if (FLAG_IS_CMDLINE(UseCompressedClassPointers)) {
warning("Compact object headers require compressed class pointers. Disabling compact object headers.");
}
FLAG_SET_DEFAULT(UseCompactObjectHeaders, false);
}
src/hotspot/share/runtime/arguments.cpp line 3130:
> 3128: }
> 3129:
> 3130: if (UseCompactObjectHeaders && !UseHeavyMonitors) {
Should this check `LockingMode`?
src/hotspot/share/runtime/globals.hpp line 133:
> 131: \
> 132: product(bool, UseCompactObjectHeaders, false, EXPERIMENTAL, \
> 133: "Use compact 64-bit object headers in 64-bit VM") \
Suggestion:
product(bool, UseCompactObjectHeaders, false, EXPERIMENTAL, \
"Use compact 64-bit object headers in 64-bit VM") \
src/hotspot/share/runtime/globals.hpp line 1067:
> 1065: "If true, error data is printed to stdout instead of a file") \
> 1066: \
> 1067: product(bool, UseHeavyMonitors, false, \
Why back to `product`?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Array.java line 87:
> 85: if (VM.getVM().isCompactObjectHeadersEnabled()) {
> 86: lengthOffsetInBytes = Oop.getHeaderSize();
> 87: } else if (VM.getVM().isCompressedKlassPointersEnabled()) {
Suggestion:
} else if (VM.getVM().isCompressedKlassPointersEnabled()) {
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Oop.java line 96:
> 94: if (VM.getVM().isCompactObjectHeadersEnabled()) {
> 95: assert(VM.getVM().isCompressedKlassPointersEnabled());
> 96: return getKlass(getMark());
Suggestion:
assert(VM.getVM().isCompressedKlassPointersEnabled());
return getKlass(getMark());
test/hotspot/jtreg/gc/g1/plab/TestPLABPromotion.java line 77:
> 75: private static final int OBJECT_SIZE_SMALL = 10;
> 76: private static final int OBJECT_SIZE_MEDIUM = 100;
> 77: private static final int OBJECT_SIZE_HIGH = (Platform.is64bit() && WhiteBox.getWhiteBox().getBooleanVMFlag("UseCompactObjectHeaders")) ? 3266 : 3250;
Please pull this into a separate flag, `private static final boolean COMPACT_OBJECT_HEADERS = ...`
test/hotspot/jtreg/runtime/FieldLayout/BaseOffsets.java line 62:
> 60:
> 61: // @0: 8 byte header, @8: int field
> 62: static final long INT_OFFSET;
What that comment is supposed to mean?
Suggestion:
// @0: 8 byte header, @8: int field
static final long INT_OFFSET;
test/jdk/java/lang/instrument/GetObjectSizeIntrinsicsTest.java line 376:
> 374: private static long expectedSmallObjSize() {
> 375: long size;
> 376: if (!Platform.is64bit() || WhiteBox.getWhiteBox().getBooleanVMFlag("UseCompactObjectHeaders")) {
`private static final boolean COMPACT_HEADERS = ...` again? Would make a negation cleaner too.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13844#pullrequestreview-1420143614
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189593168
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189596357
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189598288
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189602383
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189752465
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189754485
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189766197
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189719706
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189707883
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189707140
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189619618
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189763083
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189763521
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189626626
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189630917
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189651805
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189654298
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189750442
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189747596
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189663098
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189663671
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189673370
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189675459
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189680333
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189743882
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189684020
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189689577
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189671105
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189670590
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189672200
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189634284
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189635121
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189694294
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189696621
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189700925
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189703623
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1189702226
More information about the hotspot-dev
mailing list