RFR: 8305898: Alternative self-forwarding mechanism [v7]

[Aleksey Shipilev]

On Tue, 9 May 2023 20:54:40 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> Currently, the Serial, Parallel and G1 GCs store a pointer to self into object headers, when compaction fails, to indicate that the object has been looked at, but failed compaction into to-space. This is problematic for compact object headers ([JDK-8294992](https://bugs.openjdk.org/browse/JDK-8294992)) because it would (temporarily) over-write the crucial class information, which we need for heap parsing. I would like to propose an alternative: use the bit #3 (previously biased-locking bit) to indicate that an object is 'self-forwarded'. That preserves the crucial class information in the upper bits of the header until the full header gets restored.
>
> Roman Kennke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix asserts (again)

More comments

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

> 268: // Used by scavengers
> 269: void oopDesc::forward_to(oop p) {
> 270:   assert(p != cast_to_oop(this) || !UseAltGCForwarding, "Must not be called with self-forwarding");

Now that I had my morning coffee, I do have a question about the contract here. Can we accidentally call `oop->forward_to(compaction_point)` when `oop == compaction_point` from the compaction code? I guess that would be innocuous for the thing we want to protect against: recording the _promotion failure_, rather than the self-forwarding itself. In other words, the fact that object is self-forwarded might not exactly mean it failed the promotion, might just be a lucky coincidence?

If so, maybe this whole thing should be `oopDesc::forward_failed()` or some such, and then let the code decide how to record it, either with self-forwarding address (legacy) or with this new bit.

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

> 284:     }
> 285:     m = m.set_self_forwarded();
> 286:     assert(forwardee(m) == cast_to_oop(this), "encoding must be reversable");

Suggestion:

    assert(forwardee(m) == cast_to_oop(this), "encoding must be reversible");

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

> 313:     }
> 314:     m = m.set_self_forwarded();
> 315:     assert(forwardee(m) == cast_to_oop(this), "encoding must be reversable");

Suggestion:

    assert(forwardee(m) == cast_to_oop(this), "encoding must be reversible");

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

> 313:     }
> 314:     m = m.set_self_forwarded();
> 315:     assert(forwardee(m) == cast_to_oop(this), "encoding must be reversable");

Suggestion:

    assert(forwardee(m) == cast_to_oop(this), "encoding must be reversible");

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

PR Review: https://git.openjdk.org/jdk/pull/13779#pullrequestreview-1420088907
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189580136
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189562063
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189562326
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189562559