8305898: Alternative self-forwarding mechanism #20603

rkennke · 2024-08-15T20:52:13Z

Currently, the Serial, Parallel and G1 GCs store a pointer to self into object headers, when promotion fails, to indicate that the object has been looked at, but failed promotion. This is problematic for compact object headers (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.

A side-effect of this is that we can get rid of the machinery to preserve headers across promotion failures in Serial and G1. (Parallel GC [ab]uses the preserved-headers structure to also find all the forwarded objects for header restoration. This could be changed, I suppose, but it is not trivial.) If you prefer, I could break-out the removal of the preserved-headers stuff into a separate PR.

This is in preparation of upstreaming compact object headers, and I intend to push it only once all the parts have been approved.

Testing:

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8305898: Alternative self-forwarding mechanism (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20603/head:pull/20603
$ git checkout pull/20603

Update a local copy of the PR:
$ git checkout pull/20603
$ git pull https://git.openjdk.org/jdk.git pull/20603/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20603

View PR using the GUI difftool:
$ git pr show -t 20603

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20603.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-15T20:53:00Z

👋 Welcome back rkennke! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-08-15T20:53:13Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-08-15T20:53:38Z

@rkennke The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

rkennke · 2024-08-21T09:44:51Z

/label remove hotspot
/label add hotspot-gc

openjdk · 2024-08-21T09:46:05Z

@rkennke
The hotspot label was successfully removed.

openjdk · 2024-08-21T09:46:24Z

@rkennke
The hotspot-gc label was successfully added.

mlbridge · 2024-08-21T11:17:30Z

Webrevs

01: Full - Incremental (ba54b2d9)
00: Full (86239af3)

albertnetymk · 2024-08-21T17:45:14Z

src/hotspot/share/gc/serial/defNewGeneration.cpp

@@ -706,32 +700,27 @@ void DefNewGeneration::remove_forwarding_pointers() {
  // starts. (The mark word is overloaded: `is_marked()` == `is_forwarded()`.)
  struct ResetForwardedMarkWord : ObjectClosure {
    void do_object(oop obj) override {
-      if (obj->is_forwarded()) {
+      if (obj->is_self_forwarded()) {


Why is is_self_forwarded treated specially? I'd expect the is_forwarded case alone to be enough here.

Because I'd like self-forwarded marks not to be init-ed. Otherwise we'd have to preserve/restore them. Simply unset_self_forwarded() is enough to get them back to the original state.

Otherwise we'd have to preserve/restore them.

OK, then it's incorrect to use init_mark() for self-fwd objs.

This method is for self-fwd objs only. Can we remove the else if part? The non-self-fwd objs are essentially dead, right?

I wasn't sure about this. What happens to successfully-promoted objects? Are we sure that no references point to their from-space parts? If yes, then I'd remove the else-if part and place an assert there instead.

The else if part is needed when we later turn on UseCompactObjectHeaders, because the "normal" forwarding pointers then destroy the klass pointers, causing the object_iterate to fail to read the size of the objects.

What happens to successfully-promoted objects? ...

Successfully-forwarded objs are don't live in eden/from spaces, which are the spaces this closure is applied on. One can't have assert here, because as we iterate over eden/from spaces, we will encounter successfully-forwarded objs, but they should just be skipped.

The else if part is needed when we later turn on UseCompactObjectHeaders...

I believe so, but it should be in the UseCompactObjectHeaders PR, not this one.

Ok, thanks for the explanation!

I don't think it's needed for the UCOH part - the iterator fetched Klass*/size from forwardee if it encounters forwarded objects.

You didn't change the iterators for the Serial GC, only for Parallel. But, yes, we can deal with this in the UCOH PR.

stefank

It might not be clear to reviewers, but the suggested change forces the usage of Lightweight locking on 32-bit JVMs. I think that is OK, especially given that Legacy locking is deprecated. However, before approving this PR it would be good to know if this has been communicated to the maintainers of the affected platforms?

And with that said, I couldn't find anything in this patch that prevented 32-bit JVMs from starting with Legacy. There's only these asserts:

NOT_LP64(assert(LockingMode != LM_LEGACY, "incorrect with LM_LEGACY on 32 bit");)

rkennke · 2024-08-22T06:16:45Z

It might not be clear to reviewers, but the suggested change forces the usage of Lightweight locking on 32-bit JVMs. I think that is OK, especially given that Legacy locking is deprecated. However, before approving this PR it would be good to know if this has been communicated to the maintainers of the affected platforms?

And with that said, I couldn't find anything in this patch that prevented 32-bit JVMs from starting with Legacy. There's only these asserts:
NOT_LP64(assert(LockingMode != LM_LEGACY, "incorrect with LM_LEGACY on 32 bit");)

Right, with this change, we cannot use legacy locking on 32bit platforms anymore, because 1. the self-fwd bit would conflict with stack-locks because stack-locks are only 4-byte aligned on those platforms and 2. we no longer preserve headers around self-forwarding. No I haven't communicated this, yet.
It might be better to seperate out the removal of preserved-headers around self-forwarding, and deal with header preservation and the implications on 32-bit platforms in a separate PR, WDYT?

stefank · 2024-08-22T07:28:52Z

It might be better to seperate out the removal of preserved-headers around self-forwarding, and deal with header preservation and the implications on 32-bit platforms in a separate PR, WDYT?

I'm fine with handling it here in this PR.

rkennke · 2024-08-22T14:48:32Z

Superseding by #20677

openjdk bot added the hotspot [email protected] label Aug 15, 2024

8305898: Alternative self-forwarding mechanism

86239af

rkennke force-pushed the JDK-8305898-v4 branch from 5eb66f6 to 86239af Compare August 20, 2024 09:40

openjdk bot removed the hotspot [email protected] label Aug 21, 2024

openjdk bot added the hotspot-gc [email protected] label Aug 21, 2024

rkennke mentioned this pull request Aug 21, 2024

8305895: Implement JEP 450: Compact Object Headers (Experimental) #20640

Closed

22 tasks

rkennke marked this pull request as ready for review August 21, 2024 11:12

openjdk bot added the rfr Pull request is ready for review label Aug 21, 2024

openjdk bot mentioned this pull request Aug 21, 2024

8305896: Alternative full GC forwarding #20605

Closed

8 tasks

albertnetymk reviewed Aug 21, 2024

View reviewed changes

stefank reviewed Aug 21, 2024

View reviewed changes

Remove special handling of non-self-fwded objects

ba54b2d

rkennke mentioned this pull request Aug 22, 2024

8305895: Implement JEP 450: Compact Object Headers (Experimental) #20677

Closed

21 tasks

rkennke closed this Aug 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8305898: Alternative self-forwarding mechanism #20603

8305898: Alternative self-forwarding mechanism #20603

rkennke commented Aug 15, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 15, 2024

openjdk bot commented Aug 15, 2024

openjdk bot commented Aug 15, 2024

rkennke commented Aug 21, 2024

openjdk bot commented Aug 21, 2024

openjdk bot commented Aug 21, 2024

mlbridge bot commented Aug 21, 2024 •

edited

Loading

albertnetymk Aug 21, 2024

rkennke Aug 22, 2024

albertnetymk Aug 22, 2024

rkennke Aug 22, 2024

stefank Aug 22, 2024

albertnetymk Aug 22, 2024

rkennke Aug 22, 2024

stefank Aug 22, 2024

stefank left a comment

rkennke commented Aug 22, 2024

stefank commented Aug 22, 2024

rkennke commented Aug 22, 2024

8305898: Alternative self-forwarding mechanism #20603

8305898: Alternative self-forwarding mechanism #20603

Conversation

rkennke commented Aug 15, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewing

Webrev

bridgekeeper bot commented Aug 15, 2024

openjdk bot commented Aug 15, 2024

openjdk bot commented Aug 15, 2024

rkennke commented Aug 21, 2024

openjdk bot commented Aug 21, 2024

openjdk bot commented Aug 21, 2024

mlbridge bot commented Aug 21, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefank left a comment

Choose a reason for hiding this comment

rkennke commented Aug 22, 2024

stefank commented Aug 22, 2024

rkennke commented Aug 22, 2024

rkennke commented Aug 15, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 21, 2024 •

edited

Loading