8223312: Utilize handshakes instead of is_thread_fully_suspended

[robehn]

The main point of this change-set is to make it easier to implement S/R on top of handshakes.
Which is a prerequisite for removing _suspend_flag (which duplicates the handshake functionality).
But we also remove some complicated S/R methods.

We basically just put in everything in the handshake closure, so the diff just looks much worse than what it is.

TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
But I was unsure if I should remove now or when is_ext_suspend_completed() is removed.

Passes multiple t1-5 runs, locally it passes many jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Testing

	Linux x32	Linux x64	Windows x64	macOS x64
Build	✔️ (1/1 passed)	✔️ (5/5 passed)	✔️ (2/2 passed)	✔️ (2/2 passed)
Test (tier1)		✔️ (9/9 passed)	❌ (1/9 failed)	✔️ (9/9 passed)

Failed test task

• Windows x64 (hs/tier1 compiler)

Issue

• JDK-8223312: Utilize handshakes instead of is_thread_fully_suspended

Reviewers

• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 678e345
• Richard Reingruber (@reinrich - Committer) ⚠️ Review applies to 678e345
• Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 678e345
• Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to 678e345

Download

$ git fetch https://git.openjdk.java.net/jdk pull/729/head:pull/729
$ git checkout pull/729

[bridgekeeper]

👋 Welcome back rehn! 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]

@robehn The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

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

[mlbridge]

Webrevs

• 03: Full (c735cee)
• 02: Full (678e345)
• 01: Full - Incremental (386b930)
• 00: Full (7e77a04)

[dholmes-ora]

Hi Robbin,

IIUC the "waiting" part of wait_for_ext_suspend_completion is now implicitly handled in the handshake - correct?

Overall this seems like a great simplification.

A few minor comments below.

Thanks,
David

[openjdk]

@robehn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8223312: Utilize handshakes instead of is_thread_fully_suspended

Reviewed-by: dholmes, rrich, dcubed, eosterlund

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 2 new commits pushed to the master branch:

• cc50c8d: 8255196: Remove unused G1FullGCCompactionPoint::merge()
• ae72b52: 8255047: Add HotSpot UseDebuggerErgo flags

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

On 20/10/2020 4:18 pm, Robbin Ehn wrote:

On Tue, 20 Oct 2020 02:14:56 GMT, David Holmes <dholmes at openjdk.org> wrote:

The main point of this change-set is to make it easier to implement S/R on top of handshakes.
Which is a prerequisite for removing _suspend_flag (which duplicates the handshake functionality).
But we also remove some complicated S/R methods.

We basically just put in everything in the handshake closure, so the diff just looks much worse than what it is.

TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
But I was unsure if I should remove now or when is_ext_suspend_completed() is removed.

Passes multiple t1-5 runs, locally it passes many jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.

src/hotspot/share/runtime/thread.cpp line 579:

577: // That trace is very chatty.
578: return;
579: #else

Without the !is_wait check none of the code below line 583 is reachable now. I would remove this now.

Since only the destructor contains any actual functionality, which is now unreachable, should I remove the entire
TraceSuspendDebugBits?

Go for it! :)

Davids

[robehn]

Hi David, thanks for having a look.

Hi Robbin,

IIUC the "waiting" part of wait_for_ext_suspend_completion is now implicitly handled in the handshake - correct?

A suspended Java thread may never transition back to java, never execute any more bytecodes.
The old 'fully' suspended gives more guarantees which we need to do some operations.
When we are in a handshake, the handshake gives us those guarantees, so it is enough that thread is considered suspended.

So the answer is we wait until we are allowed to execute those operations (thread handshake safe), which is not identical with fully suspended. But fully suspended is an implementation detail, the agent only knows about suspended.

Overall this seems like a great simplification.

A few minor comments below.

Thanks,
David

[mlbridge]

Mailing list message from David Holmes on serviceability-dev:

On 20/10/2020 5:20 pm, Robbin Ehn wrote:

On Tue, 20 Oct 2020 02:22:15 GMT, David Holmes <dholmes at openjdk.org> wrote:

The main point of this change-set is to make it easier to implement S/R on top of handshakes.
Which is a prerequisite for removing _suspend_flag (which duplicates the handshake functionality).
But we also remove some complicated S/R methods.

We basically just put in everything in the handshake closure, so the diff just looks much worse than what it is.

TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
But I was unsure if I should remove now or when is_ext_suspend_completed() is removed.

Passes multiple t1-5 runs, locally it passes many jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.

src/hotspot/share/prims/jvmtiEnv.cpp line 1648:

1646: op.doit(java_thread, true);
1647: } else {
1648: Handshake::execute(&op, java_thread);

This pattern is repeated a lot - we should be able to incorporate it into the op itself by passing in `java_thread`.

My suggestion here is that we fix this in Handshake::execute() in separate RFE.

Not clear to me this is so generic that it applies to Handshake::execute
rather than being part of the operation. ??

David
-----

[robehn]

Not clear to me this is so generic that it applies to Handshake::execute
rather than being part of the operation. ??

We only have two other cases (other than JVM TI where we always do this).
This one, async exception:

    if (thread == receiver) {
      // Exception is getting thrown at self so no VM_Operation needed.
      THROW_OOP(java_throwable);
    } else {
      // Use a VM_Operation to throw the exception.
      Thread::send_async_exception(java_thread, java_throwable);
    }

And biased locking revoke where we don't do this, because it makes no sense revoking a lock if you have the bias :)
In that case we could assert we never try to revoke a self owned bias.

[dholmes-ora]

Still looks good.

[reinrich]

Hi,

this is a good change, because it is a simplification and it it makes the stack walks safe by doing them as part of a handshake.

The change conflicts with #119 though. This one is ready to be pushed since last week but was delayed due to other interferences. Would you mind me integrating #119 first? After integration it would be needed to pull 2 EscapeBarriers out of handshakes. Of course I would help do that.

Thanks, Richard.

[robehn]

Hi,

this is a good change, because it is a simplification and it it makes the stack walks safe by doing them as part of a handshake.

The change conflicts with #119 though. This one is ready to be pushed since last week but was delayed due to other interferences. Would you mind me integrating #119 first? After integration it would be needed to pull 2 EscapeBarriers out of handshakes. Of course I would help do that.

Thanks, Richard.

Hey Richard, go ahead and integrate your 119 first, I'll hold off and do the merge once you integrated.

[reinrich]

Hi,
this is a good change, because it is a simplification and it it makes the stack walks safe by doing them as part of a handshake.
The change conflicts with #119 though. This one is ready to be pushed since last week but was delayed due to other interferences. Would you mind me integrating #119 first? After integration it would be needed to pull 2 EscapeBarriers out of handshakes. Of course I would help do that.
Thanks, Richard.

Hey Richard, go ahead and integrate your 119 first, I'll hold off and do the merge once you integrated.

Thanks Robbin!

[reinrich]

Hi Robbin,

for merging master after integration of #119 I'd suggest to resolve the
conflicts by chosing the alternative from this pr and then apply
reinrich@6fa91e3
(is there a more elegant way to propose a patch?)

I successfully tested

make run-test TEST=test/jdk/com/sun/jdi/EATests.java

which also covers PopFrame and ForceEarlyReturn.

More tests are running.

For night tests of our team it is unfortunately too late.

Thanks, Richard.

[robehn]

Thanks, I'm exploring what we need to execute the EB inside the handshake.
So far I think that really needs to go in a separate PR, since it becomes really unrelated to this.... picking up your change.

Hi Robbin,

for merging master after integration of #119 I'd suggest to resolve the
conflicts by chosing the alternative from this pr and then apply
reinrich@6fa91e3
(is there a more elegant way to propose a patch?)

I successfully tested

make run-test TEST=test/jdk/com/sun/jdi/EATests.java

which also covers PopFrame and ForceEarlyReturn.

More tests are running.

For night tests of our team it is unfortunately too late.

Thanks, Richard.

[reinrich]

Thanks, I'm exploring what we need to execute the EB inside the handshake.

I want to experiment with object reallocation without referencing a frame. I think a should be possible to reallocate objects given only the corresponding compiled pc. If so, then a handshake/vm operation can fail with the request to reallocate objects at a pc. This can be done concurrently and then the handshake/vm operation can be restarted.

[robehn]

I pushed the merge. (I manage to pick up bad state in first merge, so I did a second merge to get the fixes for that)
Please have a look.

Still running test, but there were some interest in this change-set (it seem to fix an unrelated bug also) so I published it.

[robehn]

No issues in testing.

There is a bug fix which goes on top of this which people want to push.

If specially @reinrich can just look at the merge at least it would be good.

[reinrich]

The change is good.
I've only added a few comments (nothing important really).
Thanks, also for giving precedence to me ;)

[dcubed-ojdk]

I don't think I have any "must fix" comments here.

I'm going to assume that my confusion about why there
is code from @reinrich's EscapeBarrier work here is
because of the merging of conflicts...

[reinrich]

I'm going to assume that my confusion about why there
is code from @reinrich's EscapeBarrier work here is
because of the merging of conflicts...

That's correct. #119 got integrated and this pr needs to resolve a few locations because it moves code that has EscapeBarriers into handshakes. EBs cannot be executed in a handshake as they can safepoint doing heap allocations so they are moved before the handshake.

[dholmes-ora]

Incremental updates seem fine to me.

[openjdk]

@robehn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[fisk]

Just wondering why the escape barrier for force early return uses a stack depth is 0. Either that is wrong, or the escape barrier is not needed in the first place here. I think.

[fisk]

Looks good. Awesome fix IMO.

[robehn]

Passes my local testing: open/test/jdk/com/sun/jdi/EATests.java, nsk_jvmti, nsk_jdi, jdk_jdi, jck:vm.
Still running t1-t5 in test system.

I will be integrating later today, so the ZGC/EscapeBarrier issue can be resolved (which is semi-dependent on this).

Thanks @dholmes-ora, @dcubed-ojdk, @reinrich, @fisk !

[robehn]

T1-5 looked good.

/integrate

[openjdk]

@robehn Since your change was applied there have been 2 commits pushed to the master branch:

• cc50c8d: 8255196: Remove unused G1FullGCCompactionPoint::merge()
• ae72b52: 8255047: Add HotSpot UseDebuggerErgo flags

Your commit was automatically rebased without conflicts.

Pushed as commit 4634dbe.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.