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

Mon Sep 9 12:40:18 UTC 2024

On Mon, 9 Sep 2024 11:55:52 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:
> 
>  - Try to avoid lea in loadNklass (aarch64)
>  - Fix release build error

src/hotspot/share/oops/klass.hpp line 169:

> 167:                                 // contention that may happen when a nearby object is modified.
> 168:   AccessFlags _access_flags;    // Access flags. The class/interface distinction is stored here.
> 169:                                 // Some flags created by the JVM, not in the class file itself,

Suggestion:

  markWord _prototype_header;   // Used to initialize objects' header with compact headers.

Maybe some comment why this is an instance member.

src/hotspot/share/oops/objArrayKlass.inline.hpp line 74:

> 72: void ObjArrayKlass::oop_oop_iterate(oop obj, OopClosureType* closure) {
> 73:   // In this assert, we cannot safely access the Klass* with compact headers.
> 74:   assert (UseCompactObjectHeaders || obj->is_array(), "obj must be array");

If we can't safely access the `Klass*` here, why is the call to `obj->klass()` below safe?

src/hotspot/share/oops/oop.cpp line 157:

> 155: bool oopDesc::has_klass_gap() {
> 156:   // Only has a klass gap when compressed class pointers are used.
> 157:   // Except when using compact headers.

Suggestion:

  // Only has a klass gap when compressed class pointers are used and not
  // using compact headers.

(Not sure if repeating the fairly simple disjunction below makes sense, but there has been a comment before too)

src/hotspot/share/oops/oop.cpp line 230:

> 228:   // disjunct below to fail if the two comparands are computed across such
> 229:   // a concurrent change.
> 230:   return Universe::heap()->is_stw_gc_active() && klass->is_objArray_klass() && is_forwarded() && (UseParallelGC || UseG1GC);

Is this still true after the recent changes like JDK-8311163? It might be worth waiting for.

src/hotspot/share/oops/oop.hpp line 103:

> 101:   static inline void set_klass_gap(HeapWord* mem, int z);
> 102: 
> 103:   // size of object header, aligned to platform wordSize

Suggestion:

  // Size of object header, aligned to platform wordSize

Pre-existing

src/hotspot/share/oops/oop.hpp line 108:

> 106:       return sizeof(markWord) / HeapWordSize;
> 107:     } else {
> 108:       return sizeof(oopDesc) / HeapWordSize;

Suggestion:

      return sizeof(oopDesc) / HeapWordSize;

src/hotspot/share/oops/oop.hpp line 134:

> 132:   inline Klass*   forward_safe_klass(markWord m) const;
> 133:   inline size_t   forward_safe_size();
> 134:   inline void     forward_safe_init_mark();

Given the comment these methods do not seem "safe" to me. Maybe use "raw" or something to better indicate that care must be taken to use them.

Maybe the "safe" refers to use them only in "safe" contexts, but in Hotspot code iirc we use something like "raw" or "unsafe".

src/hotspot/share/oops/oop.hpp line 295:

> 293:   // this call returns null for that thread; any other thread has the
> 294:   // value of the forwarding pointer returned and does not modify "this".
> 295:   inline oop forward_to_atomic(oop p, markWord compare, atomic_memory_order order = memory_order_conservative);

Maybe add an assert in the implementation so that it is not used for self-forwarding. Same for `forward_to`.

src/hotspot/share/oops/oop.hpp line 356:

> 354:       return mark_offset_in_bytes() + sizeof(markWord) / 2;
> 355:     } else
> 356: #endif

Maybe instead of trying to calculate some random, meaningless value just use some "random" value directly?
I am fine with the existing code, but first stating directly that "any value" works here, this additional code seems to confuse the message. (Fwiw, the method is also used during Universe initialization).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750118470
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750143956
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750145460
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750150640
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750154114
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750153663
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750157781
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750159516
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1750163768