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

Stefan Karlsson stefank at openjdk.org
Fri Sep 13 09:44:19 UTC 2024 

On Fri, 13 Sep 2024 08:21:54 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:
> 
>   Hide log timestamps in test to prevent false failures

I went over the oops/ directory and added a few cleanup requests and comments.

src/hotspot/share/oops/instanceOop.hpp line 43:

> 41:     } else {
> 42:       return sizeof(instanceOopDesc);
> 43:     }

This entire function can be removed. It returns the same value as oopDesc::base_offset_in_bytes(), but in a slightly different way.

src/hotspot/share/oops/markWord.hpp line 171:

> 169:     return mask_bits(value(), lock_mask_in_place | self_fwd_mask_in_place) >= static_cast<intptr_t>(marked_value);
> 170:   }
> 171: 

Suggestion to retain code layout.
Suggestion:

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

> 27: 
> 28: #include "oops/markWord.hpp"
> 29: #include "oops/compressedOops.inline.hpp"

Suggestion:

#include "oops/compressedOops.inline.hpp"
#include "oops/markWord.hpp"

src/hotspot/share/oops/objArrayKlass.cpp line 146:

> 144: 
> 145: size_t ObjArrayKlass::oop_size(oop obj) const {
> 146:   // In this assert, we cannot safely access the Klass* with compact headers.

I would like a comment stating that this assert is turned of because size_give_klass calls oop_size on an object that might be concurrently forwarded.

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

> 156:   // Only has a klass gap when compressed class pointers are used and not
> 157:   // using compact headers.
> 158:   return UseCompressedClassPointers && !UseCompactObjectHeaders;

This comment can just be removed.

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

> 338:       // field offset. Use an offset halfway into the markWord, as the markWord is never
> 339:       // partially loaded from C2.
> 340:       return 4;

I asked around to see what people felt about dropping references to mark_offset_in_bytes(), which we know is 0. There was a request to strive to use mark_offset_in_bytes() for clarity.
Suggestion:

      return mark_offset_in_bytes() + 4;

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

> 347:   static int klass_gap_offset_in_bytes() {
> 348:     assert(has_klass_gap(), "only applicable to compressed klass pointers");
> 349:     assert(!UseCompactObjectHeaders, "don't use klass_gap_offset_in_bytes() with compact headers");

This assert is implied by `has_klass_gap()`. I don't see the need to repeat it here.

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

> 361:       return sizeof(markWord) + sizeof(Klass*);
> 362:     }
> 363:   }

Not a strong request for this PR, but there are many places that calculates almost the same thing, and it might be good to limit the number of places we do similar calculations.

I'm wondering if it wouldn't be better for readability to structure the code as follows:

  static int header_size_in_bytes() {
    if (UseCompactObjectHeaders) {
      return sizeof(markWord);
    } else if (UseCompressedClassPointers) {
      return sizeof(markWord) + sizeof(narrowKlass);
    } else {
      return sizeof(markWord) + sizeof(Klass*);
    }
  }

  // Size of object header, aligned to platform wordSize
  static int header_size() {
    return align_up(header_size_in_bytes(), HeapWordSize) / HeapWordSize;
  }
...  
  static int base_offset_in_bytes() {
    return header_size_in_bytes();
  }

src/hotspot/share/oops/oop.inline.hpp line 161:

> 159: 
> 160: void oopDesc::set_klass_gap(HeapWord* mem, int v) {
> 161:   assert(!UseCompactObjectHeaders, "don't set Klass* gap with compact headers");

We might want to consider just simplifying the function to: 

void oopDesc::set_klass_gap(HeapWord* mem, int v) {
  assert(has_klass_gap(), "precondition");
  *(int*)(((char*)mem) + klass_gap_offset_in_bytes()) = v;
}

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

> 293: // Used by scavengers
> 294: void oopDesc::forward_to(oop p) {
> 295:   assert(cast_from_oop<oopDesc*>(p) != this,

Do we really need the cast here?

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

PR Review: https://git.openjdk.org/jdk/pull/20677#pullrequestreview-2302542279
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758503206
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758482703
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758505713
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758479437
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758478106
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758472909
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758474349
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758528515
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758538380
PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1758540055

More information about the serviceability-dev mailing list