RFR: 8305895: Implementation: JEP 450: Compact Object Headers (Experimental) [v3]

[Aleksey Shipilev]

On Wed, 10 May 2023 14:23:02 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> src/hotspot/share/gc/parallel/psOldGen.cpp line 398:
>> 
>>> 396: 
>>> 397:   virtual void do_object(oop obj) {
>>> 398:     HeapWord* test_addr = cast_from_oop<HeapWord*>(obj);
>> 
>> I thought this `+1` is specifically to test that `object_start` is able to find the object header when given the interior pointer. See the `guarantee`-s in the next lines.
>
> Yes, but with compact headers, we could now have 1-word-sized objects, in which case this would fail. I am not sure how to deal with that, TBH. Maybe do the whole test only when !UseCompactObjectHeaders or when object-size is > 1?

I think we should keep the original code semantics intact, and `guarantees` might need to check stuff even with zero offset.


// With compact headers, the objects can be one-word sized.
size_t int_off = UseCompactObjectHeaders ? MIN2(1, obj->size() - 1) : 1;
HeapWord* test_addr = cast_from_oop<HeapWord*>(obj) + int_off;

>> src/hotspot/share/gc/shared/collectedHeap.cpp line 232:
>> 
>>> 230:   // With compact headers, we can't safely access the class, due
>>> 231:   // to possibly forwarded objects.
>>> 232:   if (!UseCompactObjectHeaders && is_in(object->klass_raw())) {
>> 
>> Looks good, but what this even supposed to check? `object` is not `oop` if its klass field points into Java heap? Huh? Was it some CMS shenanigan that stores something in klass word? Or is it just a glorified null check? I'll follow up on that separately.
>
> I have no idea *shrugs*

I am dealing with this separately, this is not a concern for this PR.

>> src/hotspot/share/gc/shared/collectedHeap.hpp line 312:
>> 
>>> 310: 
>>> 311:   virtual void fill_with_dummy_object(HeapWord* start, HeapWord* end, bool zap);
>>> 312:   static size_t min_dummy_object_size() {
>> 
>> Why this change?
>
> That's because oopDesc::header_size() can no longer be constexpr, because it depends on UseCompactObjectHeaders.

OK, fine.

>> src/hotspot/share/gc/shared/memAllocator.cpp line 414:
>> 
>>> 412:   // concurrent collectors.
>>> 413:   if (UseCompactObjectHeaders) {
>>> 414:     oopDesc::release_set_mark(mem, _klass->prototype_header());
>> 
>> In other cases, we do `markWord::prototype().set_narrow_klass(nk)` -- it looks safer, as we get the `markWord`-s prototype, and amend it. `_klass->prototype_header` can be removed, I think.
>
> I like _klass->prototype_header() more, and would argue that we should use that instead, here. An object's prototype mark really depends on the Klass of the object, with compact headers, and we would always get the correct prototype out of _klass->prototype_header(). Also, perhaps more importantly, the Klass::prototype_header() is useful because we can load it in generated code with a single instruction, while fetching the markWord::prototype() and amending it *at runtime* would require a whole sequence of instructions.
> 
> We only use markWord::prototype().set_narrow_klass(nk) in CDS, where the correct encoding of the narrow-klass in the header depends on the relocated Klass* location, and we couldn't safely use Klass::prototype_header().

OK, I see, makes sense.

>> src/hotspot/share/oops/oop.hpp line 124:
>> 
>>> 122:   inline size_t size_given_klass(Klass* klass);
>>> 123: 
>>> 124:   // The following set of methods is used to access the mark-word and related
>> 
>> So, these are done to avoid introducing branches on the paths where objects are definitely _not_ forwarded? Are there fewer places than where we expect forwardings? Maybe the better way would be to make all methods handle the occasional forwarding, and then provide the methods that provide the _fast-path_, like `fast_mark`, `fast_class`, etc?
>
> No, that would not work, because we have different ways to encode forwarding: full-GCs use sliding forwarding, and normal GCs use normal forwarding (with the exception of the forward-failed bit). Here, we wouldn't know which is which.

OK

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191086667
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191090403
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191090688
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191093810
PR Review Comment: https://git.openjdk.org/jdk/pull/13844#discussion_r1191096550