RFR: 8305895: Implement JEP 450: Compact Object Headers (Experimental) [v23]

Fri Sep 20 18:19:51 UTC 2024

On Thu, 19 Sep 2024 12:08:46 GMT, Roman Kennke <rkennke at 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 two additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'lilliput/JEP-450-temporary-fix-branch-2' into JDK-8305895-v4
>  - review feedback

I mostly reviewed the metaspace changes and suggest upstreaming the MetaBlock refactoring ahead of the rest of this patch.
Only one comment about the interpreter code (affecting 4 locations).

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

> 3634:     } else {
> 3635:       __ sub(r3, r3, sizeof(oopDesc));
> 3636:     }

This looks like something that could be buggy if we're not careful.  We had a pass where we cleaned up sizeof(oopDesc) once.  Can this be in oopDesc as (this is not header_size() anymore?) some function with the right name?

src/hotspot/cpu/x86/templateTable_x86.cpp line 4121:

> 4119:       __ movptr(Address(rax, rdx, Address::times_8, sizeof(oopDesc) - 1*oopSize), rcx);
> 4120:       NOT_LP64(__ movptr(Address(rax, rdx, Address::times_8, sizeof(oopDesc) - 2*oopSize), rcx));
> 4121:     }

For this and above, I'd rather oopDesc encapsulate the header_size for UseCompactObjectHeaders condition in C++ code, and never see sizeof(oopDesc).

src/hotspot/share/memory/metaspace.cpp line 799:

> 797: 
> 798:     // Set up compressed class pointer encoding.
> 799:     // In CDS=off mode, we give the JVM some leeway to choose a favorable base/shift combination.

I don't know why this comment is here.  Seems out of place.

src/hotspot/share/memory/metaspace/freeBlocks.cpp line 57:

> 55:     }
> 56:   }
> 57:   return p;

This answers my prior question.  The waste is added back to the block list for non-class-arenas as well.

src/hotspot/share/memory/metaspace/metablock.hpp line 74:

> 72: #define METABLOCKFORMATARGS(__block__)  p2i((__block__).base()), (__block__).word_size()
> 73: 
> 74: } // namespace metaspace

I am wondering if some of these metaspace changes, that is, the addition of MetaBlock could be upstreamed ahead of the CompactObjectHeaders.  Some is refactoring so that you can use the wastage to allocate into class-arena but a lot of this seems neutral to compact object headers, and would reduce this patch and allow different people to focus on just this.

src/hotspot/share/memory/metaspace/metaspaceArena.cpp line 470:

> 468: 
> 469: // Returns true if the given block is contained in this arena
> 470: // Returns true if the given block is contained in this arena

Here's the same comment twice.

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

PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2318539468
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768775590
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768781956
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768979540
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1769008437
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1769012842
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1769015008