Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257831: Suspend with handshakes #3191

Closed
wants to merge 20 commits into from
Closed

Conversation

@robehn
Copy link
Contributor

@robehn robehn commented Mar 25, 2021

A suspend request is done by handshaking thread target thread(s). When executing the handshake operation we know the target mutator thread is in a dormant state (as in safepoint safe state). We have a guarantee that it will check it's poll before leaving the dormant state. To stop the thread from leaving the the dormant state we install a second asynchronous handshake to be executed by the targeted thread. The asynchronous handshake will wait on a monitor while the thread is suspended. The target thread cannot not leave the dormant state without a resume request.

Per thread suspend requests are naturally serialized by the per thread HandshakeState lock (we can only execute one handshake at a time per thread).
Instead of having a separate lock we use this to our advantage and use HandshakeState lock for serializing access to the suspend flag and for wait/notify.

Suspend:
Requesting thread -> synchronous handshake -> target thread
Inside synchronus handshake (HandshakeState lock is locked while
executing any handshake):
- Set suspended flag
- Install asynchronous handshake

Target thread -> tries to leave dormant state -> Executes handshakes
Target only executes asynchronous handshake:
- While suspended
- Go to blocked
- Wait on HandshakeState lock

Resume:
Resuming thread:
- Lock HandshakeState lock
- Clear suspended flag
- Notify HandshakeState lock
- Unlock HandshakeState lock

The "suspend requested" flag is an optimization, without it a dormant thread could be suspended and resumed many times and each would add a new asynchronous handshake. Suspend requested flag means there is already an asynchronous suspend handshake in queue which can be re-used, only the suspend flag needs to be set.


Some code can be simplified or done in a smarter way but I refrained from doing such changes instead tried to keep existing code as is as far as possible. This concerns especially raw monitors.


Regarding the changed test, the documentation says:
"If the calling thread is specified in the request_list array, this function will not return until some other thread resumes it."

But the code:
LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
err = jvmti->SuspendThreadList(threads_count, threads, results);
...
// Allow the Main thread to inspect the result of tested threads suspension
agent_unlock(jni);

The thread will never return from SuspendThreadList until resumed, so it cannot unlock with agent_unlock().
Thus main thread is stuck forever on:
// Block until the suspender thread competes the tested threads suspension
agent_lock(jni);

And never checks and resumes the threads. So I removed that lock instead just sleep and check until all thread have the expected suspended state.


This version already contains updates after pre-review comments from @dcubed-ojdk, @pchilano, @coleenp.
(Pre-review comments here:
#2625)


Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
combinations like running with -XX:ZCollectionInterval=0.01 -
XX:ZFragmentationLimit=0.
Running above some of above concurrently (load ~240), slow debug,
etc...


Progress

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

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
$ git checkout pull/3191

Update a local copy of the PR:
$ git checkout pull/3191
$ git pull https://git.openjdk.java.net/jdk pull/3191/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3191

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3191.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 25, 2021

👋 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 openjdk bot changed the title 8257831: Suspend with handshake 8257831: Suspend with handshakes Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@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.

@robehn robehn force-pushed the robehn:SuspendInHandshake branch from 87fed73 to c47f925 Mar 25, 2021
@robehn
Copy link
Contributor Author

@robehn robehn commented Mar 25, 2021

/label remove hotspot
/label add hotspot-runtime

@openjdk openjdk bot removed the hotspot label Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@robehn
The hotspot label was successfully removed.

@robehn
Copy link
Contributor Author

@robehn robehn commented Mar 25, 2021

/label add hotspot-runtime

@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@robehn
The hotspot-runtime label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@robehn The hotspot-runtime label was already applied.

@robehn robehn marked this pull request as ready for review Mar 25, 2021
@openjdk openjdk bot added the rfr label Mar 25, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 25, 2021

@reinrich
Copy link
Contributor

@reinrich reinrich commented Mar 26, 2021

Hi Robbin,
I think the preview/pre-review #2625 of this was affected by mailing-list hick-up as I've not received a notification about it and I could not find it in the archives either. Just wanted to let you know in case you aren't already aware.
Looks promising though :)

@robehn
Copy link
Contributor Author

@robehn robehn commented Mar 26, 2021

Hi Robbin,
I think the preview/pre-review #2625 of this was affected by mailing-list hick-up as I've not received a notification about it and I could not find it in the archives either. Just wanted to let you know in case you aren't already aware.
Looks promising though :)

Hi @reinrich, I think that is because that PR was only in draft state.

Good! :)

Thanks, Robbin

@reinrich
Copy link
Contributor

@reinrich reinrich commented Mar 26, 2021

Hi Robbin,
I think the preview/pre-review #2625 of this was affected by mailing-list hick-up as I've not received a notification about it and I could not find it in the archives either. Just wanted to let you know in case you aren't already aware.
Looks promising though :)

Hi @reinrich, I think that is because that PR was only in draft state.

Ah, alright, makes sense :)

Thanks, Richard.

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

This is an elegant evolution of the suspend/resume mechanism.

It is so nice to see all the suspend-equivalent stuff go away!

src/hotspot/os/posix/signals_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/objectMonitor.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Mar 30, 2021

@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 SuspendInHandshake
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
Copy link
Contributor

@reinrich reinrich left a comment

Hi Robbin,

this is a great simplification. Excellent work! :)

Thanks, Richard.

src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/handshake.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/os.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/safepointMechanism.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/sweeper.cpp Show resolved Hide resolved
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Robbin,

This is a great piece of work with a lot of code simplifications. Unfortunately some new complexities that need to be hidden by appropriate abstractions. Lots of comments, queries and suggestions below.

Thanks,
David

src/hotspot/os/posix/signals_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiEnv.cpp Outdated Show resolved Hide resolved
src/hotspot/share/prims/jvmtiEnv.cpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiImpl.cpp Show resolved Hide resolved
src/hotspot/share/prims/jvmtiRawMonitor.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/vmStructs.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2021

@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:

8257831: Suspend with handshakes

Reviewed-by: dcubed, rrich, dholmes, pchilanomate, sspitsyn

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 18 new commits pushed to the master branch:

  • aa29784: 8265332: gtest/LargePageGtests.java OOMEs on -XX:+UseSHM cases
  • 33b6378: 8265101: Remove unnecessary functions in os*.inline.hpp
  • 28c35ae: 8259288: Debug build failure with clang-10 due to -Wimplicit-int-float-conversion
  • ca6b1b4: 8171381: [TEST_BUG] [macos] javax/swing/JPopupMenu/7156657/bug7156657.java fails on OS X
  • 9e7c748: 8265700: Regularize throws clauses in BigDecimal
  • 7116321: 8254565: JFR: Incorrect verification of mirror events
  • f45d460: 8265017: runtime/HiddenClasses/StressHiddenClasses.java timed out on Win* OCI
  • 0136c89: 8265490: Unterminated string passed to FindClass() in hotspot test
  • 325edbc: 8265450: Merge PreservedMarksSet::restore code paths
  • b337f63: 8037397: RegEx pattern matching loses character class after intersection (&&) operator
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/7146104fdaffb5037f5380eed9786fe154705150...master

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.

@openjdk openjdk bot added ready and removed merge-conflict labels Mar 31, 2021
@openjdk openjdk bot removed the ready label Apr 19, 2021
@openjdk openjdk bot added ready and removed merge-conflict labels Apr 19, 2021
@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 19, 2021

Hi David,

I have gone through everything again in detail. A few comments on comments and a couple of details I'm still not really clear on.

I hope I made things more clear.

A couple of small renaming requests, but otherwise this is good to go - with the caveat of a future RFE to clear up the suspend-while-locking implementations so things are more cleanly encapsulated.

Yes, good.

Fixed what I could in this commit, also fixed merged conflict.

Thanks, Robbin

Thanks,
David

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

One minor nit from a review of incremental v10.
Still thumbs up.

JavaThread* thread_current = thr->as_Java_thread();
assert(thread_current == Thread::current(), "Must be self executed.");
thread_current->handshake_state()->do_self_suspend();
Comment on lines 634 to 636

This comment has been minimized.

@dcubed-ojdk

dcubed-ojdk Apr 19, 2021
Member

nit - When you know it is the "current" thread, @dholmes-ora has been
using just current and not current_thread or thread_current.

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 20, 2021

Thanks for all input, done done !? :)

Re-running test.

Copy link
Contributor

@pchilano pchilano left a comment

Latest version LGTM!

Thanks,
Patricio

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good.

JavaThread* target = thr->as_Java_thread();
target->handshake_state()->do_self_suspend();
JavaThread* current = thr->as_Java_thread();
assert(current == Thread::current(), "Must be self executed.");

This comment has been minimized.

@dholmes-ora

dholmes-ora Apr 21, 2021
Member

Possibly overkill given do_self_suspend() has a similar assertion.

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 21, 2021

Latest version LGTM!

Thanks,
Patricio

Thanks

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 21, 2021

Looks good.

Thanks

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 21, 2021

I'll push tomorrow morning and fill RFE to fix the manual transitions in OM/rawmonitor.
Please object if there is some major that cannot be fixed as a follow-up.

Thanks for all the feedback!

Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Reviewed the v11 and v12 incrementals.
Still a thumbs up.

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 21, 2021

Reviewed the v11 and v12 incrementals.
Still a thumbs up.

Thank you!

Copy link
Contributor

@reinrich reinrich left a comment

I've followed the discussion and the increments. Still looks very good to me 👍

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 21, 2021

I've followed the discussion and the increments. Still looks very good to me

Thank you!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Approving again for good measure. :)

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 22, 2021

Approving again for good measure. :)

Thank you!

@robehn
Copy link
Contributor Author

@robehn robehn commented Apr 22, 2021

/integrate

@openjdk openjdk bot closed this Apr 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

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

  • 28af31d: 8263668: Update java.time to use instanceof pattern variable
  • a93d911: 8265607: Avoid decrementing when no Symbol was created in ~SignatureStream
  • aa29784: 8265332: gtest/LargePageGtests.java OOMEs on -XX:+UseSHM cases
  • 33b6378: 8265101: Remove unnecessary functions in os*.inline.hpp
  • 28c35ae: 8259288: Debug build failure with clang-10 due to -Wimplicit-int-float-conversion
  • ca6b1b4: 8171381: [TEST_BUG] [macos] javax/swing/JPopupMenu/7156657/bug7156657.java fails on OS X
  • 9e7c748: 8265700: Regularize throws clauses in BigDecimal
  • 7116321: 8254565: JFR: Incorrect verification of mirror events
  • f45d460: 8265017: runtime/HiddenClasses/StressHiddenClasses.java timed out on Win* OCI
  • 0136c89: 8265490: Unterminated string passed to FindClass() in hotspot test
  • ... and 10 more: https://git.openjdk.java.net/jdk/compare/7146104fdaffb5037f5380eed9786fe154705150...master

Your commit was automatically rebased without conflicts.

Pushed as commit 86bd44f.

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

@robehn robehn deleted the robehn:SuspendInHandshake branch Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment