On Mon, 9 Sep 2024 17:45:47 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> This is the main body of the JEP 450: Compact Object Headers (Experimental).
>>
>> It is also a follow-up to #20640, which now also includes (and supersedes)
>> #20603 and #20605, plus the Tiny Class-Pointers parts that have been
>> previously missing.
>>
>> Main changes:
>> - Introduction of the (experimental) flag UseCompactObjectHeaders. All
>> changes in this PR are protected by this flag. The purpose of the flag is to
>> provide a fallback, in case that users unexpectedly observe problems with
>> the new implementation. The intention is that this flag will remain
>> experimental and opt-in for at least one release, then make it on-by-default
>> and diagnostic (?), and eventually deprecate and obsolete it. However, there
>> are a few unknowns in that plan, specifically, we may want to further
>> improve compact headers to 4 bytes, we are planning to enhance the Klass*
>> encoding to support virtually unlimited number of Klasses, at which point we
>> could also obsolete UseCompressedClassPointers.
>> - The compressed Klass* can now be stored in the mark-word of objects. In
>> order to be able to do this, we are add some changes to GC forwarding (see
>> below) to protect the relevant (upper 22) bits of the mark-word. Significant
>> parts of this PR deal with loading the compressed Klass* from the mark-word.
>> 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.
>> - Self-forwarding in GCs (which is used to deal with promotion failure) now
>> uses a bit to indicate 'self-forwarding'. This is needed to preserve the
>> crucial Klass* bits in the header. This also allows to get rid of
>> preserved-header machinery in SerialGC and G1 (Parallel GC abuses
>> preserved-marks to also find all other relevant oops).
>> - Full GC forwarding now uses an encoding similar to compressed-oops. We
>> have 40 bits for that, and can encode up to 8TB of heap. When exceeding 8TB,
>> we turn off UseCompressedClassPointers (except in ZGC, which doesn't use the
>> GC forwarding at all).
>> - 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 now store their length at offset 8.
>> - 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. Some build machinery is added so that
>> _co...
>
> Roman Kennke has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Print as warning when UCOH doesn't match in CDS archive
> - Improve initialization of mark-word in CDS ArchiveHeapWriter
> - Simplify getKlass() in SA
> - Simplify oopDesc::init_mark()
> - Get rid of forward_safe_* methods
> - GCForwarding touch-ups
I reviewed the oops code so far.
src/hotspot/share/oops/compressedKlass.cpp line 116:
> 114: _range = end - _base;
> 115:
> 116: DEBUG_ONLY(assert_is_valid_encoding(addr, len, _base, _shift);)
Can you refactor so the aarch64 path runs this same code without duplication?
src/hotspot/share/oops/klass.hpp line 173:
> 171:
> 172: markWord _prototype_header; // Used to initialize objects' header
> 173:
I think you should move this up after ClassLoaderData, as there might be an
alignment gap (you can run pahole to check).
src/hotspot/share/oops/klass.hpp line 718:
> 716:
> 717: markWord prototype_header() const {
> 718: assert(UseCompactObjectHeaders, "only use with compact object
> headers");
Should this unconditionally return _prototype_header since it's initialized to
markWord::prototype_header(), or would that decrease performance for the
non-compact headers case?
src/hotspot/share/oops/klass.inline.hpp line 54:
> 52: }
> 53:
> 54: inline void Klass::set_prototype_header(markWord header) {
Can you put a comment that this is only used when dumping the archive? Because
otherwise the Klass::_prototype_header field should always be initialized to
the right thing (either with Klass encoded or as markWord::protoytpe_header())
and doesn't change.
src/hotspot/share/oops/markWord.inline.hpp line 90:
> 88: ShouldNotReachHere();
> 89: return markWord();
> 90: #endif
Is the ifdef _LP64 necessary, since UseCompactObjectHeaders should always be
false for 32 bits?
src/hotspot/share/oops/oop.inline.hpp line 90:
> 88: } else {
> 89: return markWord::prototype();
> 90: }
Could this be unconditional since prototoype_header is initialized for all
Klasses?
src/hotspot/share/oops/typeArrayKlass.cpp line 175:
> 173: size_t TypeArrayKlass::oop_size(oop obj) const {
> 174: // In this assert, we cannot safely access the Klass* with compact
> headers.
> 175: assert(UseCompactObjectHeaders || obj->is_typeArray(),"must be a type
> array");
Why not? I think I'm missing something. Klass should be in the markWord and
that should be ok (?)
-------------
PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2290316150
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750529270
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750727211
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750730078
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750736547
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750739441
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750842383
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750721069
