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

[Aleksey Shipilev]

On Mon, 8 May 2023 18:42:36 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 with a new target base due to a merge or a rebase. The pull request now contains ten commits:
> 
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Use forwardee() in forward_to_atomic() method
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Merge branch 'JDK-8305896' into JDK-8305898
>  - Replace uses of decode_pointer() with forwardee()
>  - 8305898: Alternative self-forwarding mechanism

Looks okay at the first glance, comments:

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

> 269: void oopDesc::forward_to(oop p) {
> 270:   markWord m = markWord::encode_pointer_as_mark(p);
> 271:   assert(forwardee(m) == p, "encoding must be reversable");

Suggestion:

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

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

> 276:   if (UseAltGCForwarding) {
> 277:     markWord m = mark();
> 278:     // If mark is displaced, we need to preserve real header during GC.

Suggestion:

    // If mark is displaced, we need to preserve the real header during GC.

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

> 302: 
> 303: oop oopDesc::forward_to_self_atomic(markWord compare, atomic_memory_order order) {
> 304:   if (UseAltGCForwarding) {

Do you want to assert in `oopDesc::forward_to` and `oopDesc::forward_to_atomic` that they are not called with self-forwarding arguments?

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

> 304:   if (UseAltGCForwarding) {
> 305:     markWord m = compare;
> 306:     // If mark is displaced, we need to preserve real header during GC.

Suggestion:

    // If mark is displaced, we need to preserve the real header during GC.

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

> 320:     }
> 321:   } else {
> 322:     return forward_to_atomic(oop(this), compare, order);

Suggestion:

    return forward_to_atomic(cast_to_oop(this), compare, order);

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

> 327:   assert(header.is_marked(), "only decode when actually forwarded");
> 328:   if (header.self_forwarded()) {
> 329:     assert(UseAltGCForwarding, "Only use self-fwd bits when using alt GC forwarding");

This assert looks excessive, as `self_forwarded` asserts the same?

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

> 330:     return cast_to_oop(this);
> 331:   } else {
> 332:     return cast_to_oop(header.decode_pointer());

I think this path misses the original assert:


assert(is_forwarded(), "only decode when actually forwarded");

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

PR Review: https://git.openjdk.org/jdk/pull/13779#pullrequestreview-1419298697
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189037701
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189030583
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189040413
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189034503
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189038639
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189041662
PR Review Comment: https://git.openjdk.org/jdk/pull/13779#discussion_r1189035731