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

[Stefan Karlsson]

On Fri, 4 Oct 2024 11:15:37 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 with a new target base due to a merge or a rebase. The pull request now contains 76 commits:
> 
>  - Merge remote-tracking branch 'rkennke/JDK-8305895-v4' into JDK-8305895-v4
>  - Revert "Disable TestSplitPacks::test4a, failing on aarch64"
>    
>    This reverts commit 059b1573db26d1d2902ca6dadc8413f445234c2a.
>  - Simplify object init code in interpreter
>  - Disable some vectorization tests that fail with +UCOH and UseSSE<=3
>  - Fix for CDSPluginTest.java
>  - Merge tag 'jdk-24+18' into JDK-8305895-v4
>    
>    Added tag jdk-24+18 for changeset 19642bd3
>  - Disable TestSplitPacks::test4a, failing on aarch64
>  - @robcasloz review comments
>  - Improve CollectedHeap::is_oop()
>  - Allow LM_MONITOR on 32-bit platforms
>  - ... and 66 more: https://git.openjdk.org/jdk/compare/19642bd3...8742f3c1

I realize that some of my comments was stuck as drafts and had not been published. I took an extra pass over the gc/ and most of oops/ with the intention to approve those parts. However, I see that the comment about `fill_dense_prefix_end` and `MinObjectAlignment` has not been addressed. I don't know if that change is correct, so it would be good to get that scrutinized.

src/hotspot/share/gc/shared/collectedHeap.cpp line 223:

> 221: }
> 222: 
> 223: bool klass_is_sane(oop object) {

Should at least be static. We might also want to keep reporting errors if a klass pointer is null when we don't have a forwarded object:

static bool klass_is_sane(oop object) {
  if (UseCompactObjectHeaders) {
    // With compact headers, we can't safely access the Klass* when
    // the object has been forwarded, because non-full-GC-forwarding
    // distinction between Full-GC and regular GC forwarding.
    markWord mark = object->mark();
    if (mark.is_forwarded()) {
      // We can't access the Klass*. We optimistically assume that
      // it is ok. This happens very rarely.
      return true;
    }

    return Metaspace::contains(mark.klass_without_asserts());
  }

  return Metaspace::contains(object->klass_without_asserts());
}

src/hotspot/share/oops/compressedKlass.cpp line 28:

> 26: #include "logging/log.hpp"
> 27: #include "memory/metaspace.hpp"
> 28: #include "oops/klass.hpp"

Is this include really needed or could this be reverted klass.hpp? If it is needed is should be moved to after compressedKlass.inline.hpp.

src/hotspot/share/oops/compressedKlass.cpp line 31:

> 29: #include "oops/compressedKlass.inline.hpp"
> 30: #include "runtime/globals.hpp"
> 31: #include "runtime/java.hpp"

Do you remember why this was added?

src/hotspot/share/oops/markWord.cpp line 35:

> 33: STATIC_ASSERT(markWord::klass_shift + markWord::klass_bits == 64);
> 34: // The hash (preceding nKlass) shall be a direct neighbor but not interleave
> 35: STATIC_ASSERT(markWord::klass_shift == markWord::hash_bits + markWord::hash_shift);

The code is not consistent in it usage of the name for the klass bits. Here it says `nKlass` in the comment, but the fields are named `klass`. Maybe just change the comment to says `(preceding klass bits)`.

Note that the term `nklass` is not prevalent in the code base, but with this patch its starting to get a foot hold. It might be good to figure out what we do want to call these in field names and variables to at least a little bit more consistency in the code base. Currently we have `nklass`, `nKlass` `nk`, `narrow_klass`.

 In other places we have functions that are named `set_narrow_klass`, but the field is called `nklass` and other places call it `nk`. It would be good to stay consistent with the naming. FWIW, nklass has very little precedence in the code, so cleaning that away might be easiest.thing is to clean out all usages of nklass, because it isn't a

src/hotspot/share/oops/markWord.inline.hpp line 35:

> 33:   assert(UseCompactObjectHeaders, "only used with compact object headers");
> 34:   const narrowKlass nk = value() >> klass_shift;
> 35:   return narrowKlass(value() >> klass_shift);

This sets up an unused variable.
```suggestion 
  const narrowKlass nk = value() >> klass_shift;
  return narrowKlass(value() >> klass_shift);

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

PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2331450326
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1777180846
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1789757910
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1789759027
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1789787634
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1789790323