On Tue, 10 Sep 2024 19:11:30 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 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
