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

Wed Sep 11 14:00:17 UTC 2024

On Tue, 10 Sep 2024 19:11:30 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 one additional commit since the last revision:
> 
>   Fix FullGCForwarding initialization

Hi,

Me and @caspernorrbin are reviewing the Metaspace changes (so anything in the `memory` and `metaspace` folders). We have found minor improvements that can be made and some nits, but the code over all looks OK. We are finishing up a first round of review now, and will have a second one.

Thank you for your hard work and your patience with the review process.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 87:

> 85:         klass_alignment_words,
> 86:         "class arena");
> 87:   }

As per my comment in the header file, change the code to this:

```c++
if (class_context != nullptr) {
  // ... Same as in PR
} else {
  _class_space_arena = _non_class_space_arena;
}

src/hotspot/share/memory/classLoaderMetaspace.cpp line 115:

> 113:   if (wastage.is_nonempty()) {
> 114:     non_class_space_arena()->deallocate(wastage);
> 115:   }

This code reads a bit strangely. I understand *what* it tries to do. It tries to give back any wasted memory from either the class space arena *or* the non class space arena to the non class space arena's freelist. I assume that we do this since any wastage is presumably too small to be used by our new 22-bit class pointers. However, this context will be lost on future readers. It should have at least a comment in the `if (wastage.is_nonempty())` clause explaining what we expect should happen and why. For example:

```c++
// Any wasted memory is presumably too small for any class.
// Therefore, give it back to the non-class space arena's free list.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 118:

> 116: #ifdef ASSERT
> 117:   if (result.is_nonempty()) {
> 118:     const bool in_class_arena = class_space_arena() != nullptr ? class_space_arena()->contains(result) : false;

Unnecessary nullptr check if you take my suggestion, or you should switch to `have_class_space_arena`.

src/hotspot/share/memory/classLoaderMetaspace.cpp line 165:

> 163:   MetaBlock bl(ptr, word_size);
> 164:   // If the block would be reusable for a Klass, add to class arena, otherwise to
> 165:   // then non-class arena.

Nit: spelling, "the"

src/hotspot/share/memory/classLoaderMetaspace.hpp line 81:

> 79:   metaspace::MetaspaceArena* class_space_arena() const       { return _class_space_arena; }
> 80: 
> 81:   bool have_class_space_arena() const { return _class_space_arena != nullptr; }

This is unnecessary. Instead of having this and having to remember to check for nullness each time, just change the `_class_space_arena` to point to the same arena as the `_non_class_space_arena` does when we run with `-XX:-UseCompressedClassPointers`

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

> 654:     // Adjust size of the compressed class space.
> 655: 
> 656:     const size_t res_align = reserve_alignment();

Can you change the name to `root_chunk_size`?

src/hotspot/share/memory/metaspace.hpp line 112:

> 110:   static size_t max_allocation_word_size();
> 111: 
> 112:   // Minimum allocation alignment, in bytes. All MetaData shall be aligned correclty

Nit: Spelling, "correctly"

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

> 46: 
> 47:   MetaWord* base() const { return _base; }
> 48:   const MetaWord* end() const { return _base + _word_size; }

`assert(is_nonempty())`

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

> 49:   size_t word_size() const { return _word_size; }
> 50:   bool is_empty() const { return _base == nullptr; }
> 51:   bool is_nonempty() const { return _base != nullptr; }

Can `_base == nullptr` but `_word_size != 0`?

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

> 50:   bool is_empty() const { return _base == nullptr; }
> 51:   bool is_nonempty() const { return _base != nullptr; }
> 52:   void reset() { _base = nullptr; _word_size = 0; }

Is this function really necessary? According to my IDE it's only used in tests and even then the `MetaBlock` isn't used afterwards (so it has no effect).

src/hotspot/share/memory/metaspace/metaspaceArena.hpp line 44:

> 42: class FreeBlocks;
> 43: 
> 44: struct ArenaStats;

Nit: Sort?

src/hotspot/share/memory/metaspace/metaspaceArena.hpp line 84:

> 82:   // between threads and needs to be synchronized in CLMS.
> 83: 
> 84:   const size_t _allocation_alignment_words;

Nit: Document this? All other members are documented.

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

PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2296528491
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754335269
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754398993
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754343513
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754459464
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754330432
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754619023
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754508321
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754142822
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754142098
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754153662
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754192464
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1754197251