Re: RFR: 8257831: Suspend with handshakes [v14]
On Thu, 22 Apr 2021 09:36:26 GMT, David Holmes wrote: > Approving again for good measure. :) Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 07:59:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 20 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
Approving again for good measure. :)
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 13:25:53 GMT, Richard Reingruber wrote: > I've followed the discussion and the increments. Still looks very good to me Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 07:59:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 20 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
I've followed the discussion and the increments. Still looks very good to me 👍
-
Marked as reviewed by rrich (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 13:17:02 GMT, Daniel D. Daugherty wrote: > Reviewed the v11 and v12 incrementals. > Still a thumbs up. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 07:59:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 20 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
Reviewed the v11 and v12 incrementals.
Still a thumbs up.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
On Wed, 21 Apr 2021 07:59:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 20 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - ... and 10 more:
> https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
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!
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v14]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 20 commits:
- Merge branch 'master' into SuspendInHandshake
- Merge branch 'master' into SuspendInHandshake
- Removed double tty lock unlocks
- Fixed comment from Dan and fixed name of handshake
- Merge branch 'master' into SuspendInHandshake
- Fixed DavidH second review
- Fixed nits
- Merge branch 'master' into SuspendInHandshake
- Review fixes 4
- Fixed flag undef dep + spelling error
- ... and 10 more: https://git.openjdk.java.net/jdk/compare/7146104f...ec344661
-
Changes: https://git.openjdk.java.net/jdk/pull/3191/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=13
Stats: 1349 lines in 40 files changed: 271 ins; 882 del; 196 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v13]
On Tue, 20 Apr 2021 18:09:07 GMT, Patricio Chilano Mateo wrote: > Latest version LGTM! > > Thanks, > Patricio Thanks - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v13]
On Wed, 21 Apr 2021 04:32:36 GMT, David Holmes wrote: > Looks good. Thanks - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Wed, 21 Apr 2021 04:39:35 GMT, David Holmes wrote: >> Borrowing the HandshakeState lock does create an artificial coupling here. >> I'd prefer to see this API in a more natural place with friendship used to >> access the mechanism as needed. Future cleanup though. > > Also I think you'd have a lost-wakeup problem if two locks were involved. You are correct, above those not work as is. We would need to protect the suspend flag with SomeOtherLock, thus also needing to lock SomeOtherLock when suspending. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Wed, 21 Apr 2021 04:36:07 GMT, David Holmes wrote:
>> When going to blocked inside the async handshake we **must** unlock the
>> HandshakeState lock.
>> Thus _lock.wait() unlocks and gives us something to notify.
>> We could do:
>>
>> _lock.unlock();
>> {
>> MutexLocker(SomeOtherLock) ml;
>> SomeOtherLock.wait();
>> }
>> _lock.lock();
>>
>>
>> Another alternative is to create a friend class which uses the
>> HandshakeState lock and having the API there instead.
>
> Borrowing the HandshakeState lock does create an artificial coupling here.
> I'd prefer to see this API in a more natural place with friendship used to
> access the mechanism as needed. Future cleanup though.
Also I think you'd have a lost-wakeup problem if two locks were involved.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 07:13:57 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.hpp line 133:
>>
>>> 131: // check the _lock, if held go to slow path.
>>> 132: // Since the handshakee is unsafe if _lock gets locked after this
>>> check
>>> 133: // we know another thread cannot process any handshakes.
>>
>> I can't quite understand this comment. I'm not sure what thread is calling
>> this method and when, in relation to what the handshakee may be doing.
>
> The handshakee is in a safe state, e.g. blocked and does:
> Point A: set_thread_state_fence(_thread_blocked_trans);
> ...
> Point B: _lock.is_locked()
>
> While the processor thread does:
> Point X: _lock.try_lock();
> ...
> Point Z: thread->thread_state();
>
> If point B is passed with a non-locked lock, point Z is guaranteed to see an
> unsafe thread and will not start to process ay handshakes.
>
> Is that make sense, maybe the comment make more sense ? :)
The comment makes more sense now :)
>> src/hotspot/share/runtime/handshake.hpp line 157:
>>
>>> 155: Thread* active_handshaker() const { return _active_handshaker; }
>>> 156:
>>> 157: // Suspend/resume support
>>
>> Taking a step back I can't help but think that this is entirely the wrong
>> place to have the suspend/resume API and supporting code. It is only here
>> because we re-use the HandshakeState _lock monitor right? If we introduced a
>> new thread-suspension monitor instead then this code would all reside
>> somewhere else - probably in the JavaThread class.
>
> When going to blocked inside the async handshake we **must** unlock the
> HandshakeState lock.
> Thus _lock.wait() unlocks and gives us something to notify.
> We could do:
>
> _lock.unlock();
> {
> MutexLocker(SomeOtherLock) ml;
> SomeOtherLock.wait();
> }
> _lock.lock();
>
>
> Another alternative is to create a friend class which uses the HandshakeState
> lock and having the API there instead.
Borrowing the HandshakeState lock does create an artificial coupling here. I'd
prefer to see this API in a more natural place with friendship used to access
the mechanism as needed. Future cleanup though.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v13]
On Tue, 20 Apr 2021 06:48:47 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 19 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - ... and 9 more:
> https://git.openjdk.java.net/jdk/compare/f1d4ae6c...56dc3df0
Looks good.
src/hotspot/share/runtime/handshake.cpp line 633:
> 631: void do_thread(Thread* thr) {
> 632: JavaThread* current = thr->as_Java_thread();
> 633: assert(current == Thread::current(), "Must be self executed.");
Possibly overkill given do_self_suspend() has a similar assertion.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v13]
On Tue, 20 Apr 2021 06:48:47 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 19 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Removed double tty lock unlocks
> - Fixed comment from Dan and fixed name of handshake
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - ... and 9 more:
> https://git.openjdk.java.net/jdk/compare/f1d4ae6c...56dc3df0
Latest version LGTM!
Thanks,
Patricio
-
Marked as reviewed by pchilanomate (Committer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Mon, 19 Apr 2021 18:19:05 GMT, Patricio Chilano Mateo wrote: >> Is this worth a comment above the `break_tty_lock_for_safepoint()` call? > > I also thought we could remove this since we are already releasing it in the > ThreadInVMForHandshake constructor above. Yes, sorry, removed. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v12]
On Mon, 19 Apr 2021 08:22:35 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/e390e550...2c652f94
Thanks for all input, done done !? :)
Re-running test.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v13]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 19 commits:
- Merge branch 'master' into SuspendInHandshake
- Removed double tty lock unlocks
- Fixed comment from Dan and fixed name of handshake
- Merge branch 'master' into SuspendInHandshake
- Fixed DavidH second review
- Fixed nits
- Merge branch 'master' into SuspendInHandshake
- Review fixes 4
- Fixed flag undef dep + spelling error
- Obsolete unused flags
- ... and 9 more: https://git.openjdk.java.net/jdk/compare/f1d4ae6c...56dc3df0
-
Changes: https://git.openjdk.java.net/jdk/pull/3191/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=12
Stats: 1349 lines in 40 files changed: 271 ins; 882 del; 196 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v12]
On Mon, 19 Apr 2021 08:22:35 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/e390e550...2c652f94
One minor nit from a review of incremental v10.
Still thumbs up.
src/hotspot/share/runtime/handshake.cpp line 636:
> 634: JavaThread* thread_current = thr->as_Java_thread();
> 635: assert(thread_current == Thread::current(), "Must be self
> executed.");
> 636: thread_current->handshake_state()->do_self_suspend();
nit - When you know it is the "current" thread, @dholmes-ora has been
using just `current` and not `current_thread` or `thread_current`.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Fri, 9 Apr 2021 15:25:16 GMT, Daniel D. Daugherty wrote: >> If we process the async suspension handshake we can go to safepoint. >> And before safepoint we must drop the tty lock. > > Is this worth a comment above the `break_tty_lock_for_safepoint()` call? I also thought we could remove this since we are already releasing it in the ThreadInVMForHandshake constructor above. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v12]
On Mon, 19 Apr 2021 08:22:35 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Fixed DavidH second review
> - Fixed nits
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/e390e550...2c652f94
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
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v12]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 16 commits:
- Merge branch 'master' into SuspendInHandshake
- Fixed DavidH second review
- Fixed nits
- Merge branch 'master' into SuspendInHandshake
- Review fixes 4
- Fixed flag undef dep + spelling error
- Obsolete unused flags
- Review fixes 3
- Merge branch 'master' into SuspendInHandshake
- Review fixes 2
- ... and 6 more: https://git.openjdk.java.net/jdk/compare/e390e550...2c652f94
-
Changes: https://git.openjdk.java.net/jdk/pull/3191/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=11
Stats: 1351 lines in 40 files changed: 273 ins; 882 del; 196 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v11]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request incrementally with one additional
commit since the last revision:
Fixed DavidH second review
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3191/files
- new: https://git.openjdk.java.net/jdk/pull/3191/files/d6e091da..5a4d52ce
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=10
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=09-10
Stats: 13 lines in 2 files changed: 1 ins; 1 del; 11 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 06:07:54 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed nits
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Thread.java line
> 83:
>
>> 81: }
>> 82:
>> 83: public boolean isExtSuspended() {
>
> A new comment to avoid the "resolved" conversation. I'm still not sure you
> can just delete this API from the SA. @sspitsyn do you know what the
> obligations are with respect to the S.A here?
The SA do not have the HandshakeState class implemented, so there is a lot a
work to make this flag work.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 04:47:33 GMT, David Holmes wrote: >> Robbin Ehn has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed nits > > src/hotspot/share/runtime/mutex.hpp line 70: > >> 68:tty= access + 2, >> 69:special= tty+ 3, >> 70:oopstorage = special+ 3, > > Why +3? I'm assuming to keep the same numeric value as before, but that > doesn't seem necessary and just seems to add to the mystery of why these > ranks are defined in such a strange way. Yes I wanted to keep the numeric rank. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 05:48:44 GMT, David Holmes wrote: > It is still not at all clear to me what suspended has to do with the execution > of this method. The new code just ignores thread suspension. The caller enters a safe state. It can be suspended iff safe, so the old code checked for suspension. With the new code suspension is part of handshake processing, so it is sufficient to check for pending handshakes. > It seems to me that the old code could also have ignored suspension if the > checks in handle_special_runtime_exit_condition had be reordered. In that case JavaThread::java_suspend_self_with_safepoint_check() would have to be changed to check for `Thread::is_obj_deopt_suspend()` and handle it. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 04:43:15 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed nits
>
> src/hotspot/share/runtime/handshake.hpp line 157:
>
>> 155: Thread* active_handshaker() const { return _active_handshaker; }
>> 156:
>> 157: // Suspend/resume support
>
> Taking a step back I can't help but think that this is entirely the wrong
> place to have the suspend/resume API and supporting code. It is only here
> because we re-use the HandshakeState _lock monitor right? If we introduced a
> new thread-suspension monitor instead then this code would all reside
> somewhere else - probably in the JavaThread class.
When going to blocked inside the async handshake we **must** unlock the
HandshakeState lock.
Thus _lock.wait() unlocks and gives us something to notify.
We could do:
_lock.unlock();
{
MutexLocker(SomeOtherLock) ml;
SomeOtherLock.wait();
}
_lock.lock();
Another alternative is to create a friend class which uses the HandshakeState
lock and having the API there instead.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 04:34:00 GMT, David Holmes wrote: >> Robbin Ehn has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed nits > > src/hotspot/share/runtime/handshake.hpp line 134: > >> 132: // Since the handshakee is unsafe if _lock gets locked after this >> check >> 133: // we know another thread cannot process any handshakes. >> 134: // Now we can check queue if there is anything we should process. > > // Now we can check the queue to see if there is anything we should process. Fixed - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Mon, 19 Apr 2021 04:19:28 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed nits
>
> src/hotspot/share/runtime/handshake.cpp line 630:
>
>> 628: // This is the closure that prevents a suspended JavaThread from
>> 629: // escaping the suspend request.
>> 630: class ThreadSuspensionHandshake : public AsyncHandshakeClosure {
>
> I still find ThreadSuspensionHandShake versus SuspendThreadHandshake to be
> too similarly named and with no obvious way to remember which one is which.
> Perhaps this one could be called ThreadSelfSuspensionHandshake instead?
Fixed
> src/hotspot/share/runtime/handshake.cpp line 636:
>
>> 634: JavaThread* target = thr->as_Java_thread();
>> 635: target->handshake_state()->do_self_suspend();
>> 636: }
>
> As this must be the current thread we are dealing with can we rename the
> variables to indicate that.
Fixed
> src/hotspot/share/runtime/handshake.hpp line 128:
>
>> 126: // must take slow path, process_by_self(), if _lock is held.
>> 127: bool should_process() {
>> 128: // When doing thread suspension the holder of the _lock
>
> Potentially this could be any async handshake operation - right? It seems odd
> to talk specifically about thread suspension in the core of the handshake
> code.
Changed
> src/hotspot/share/runtime/handshake.hpp line 131:
>
>> 129: // can add an asynchronous handshake to queue.
>> 130: // To make sure it is seen by the handshakee, the handshakee must
>> first
>> 131: // check the _lock, if held go to slow path.
>
> ..., _and_ if held ...
Fixed
> src/hotspot/share/runtime/handshake.hpp line 133:
>
>> 131: // check the _lock, if held go to slow path.
>> 132: // Since the handshakee is unsafe if _lock gets locked after this
>> check
>> 133: // we know another thread cannot process any handshakes.
>
> I can't quite understand this comment. I'm not sure what thread is calling
> this method and when, in relation to what the handshakee may be doing.
The handshakee is in a safe state, e.g. blocked and does:
Point A: set_thread_state_fence(_thread_blocked_trans);
...
Point B: _lock.is_locked()
While the processor thread does:
Point X: _lock.try_lock();
...
Point Z: thread->thread_state();
If point B is passed with a non-locked lock, point Z is guaranteed to see an
unsafe thread and will not start to process ay handshakes.
Is that make sense, maybe the comment make more sense ? :)
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Fri, 16 Apr 2021 06:44:06 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed nits
Hi Robbin,
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.
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.
Thanks,
David
src/hotspot/share/runtime/handshake.cpp line 630:
> 628: // This is the closure that prevents a suspended JavaThread from
> 629: // escaping the suspend request.
> 630: class ThreadSuspensionHandshake : public AsyncHandshakeClosure {
I still find ThreadSuspensionHandShake versus SuspendThreadHandshake to be too
similarly named and with no obvious way to remember which one is which. Perhaps
this one could be called ThreadSelfSuspensionHandshake instead?
src/hotspot/share/runtime/handshake.cpp line 636:
> 634: JavaThread* target = thr->as_Java_thread();
> 635: target->handshake_state()->do_self_suspend();
> 636: }
As this must be the current thread we are dealing with can we rename the
variables to indicate that.
src/hotspot/share/runtime/handshake.hpp line 128:
> 126: // must take slow path, process_by_self(), if _lock is held.
> 127: bool should_process() {
> 128: // When doing thread suspension the holder of the _lock
Potent
Re: RFR: 8257831: Suspend with handshakes [v10]
On Fri, 16 Apr 2021 06:44:06 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed nits
Reviewed the v09 incremental. Still thumbs up.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
On Fri, 16 Apr 2021 06:44:06 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed nits
Marked as reviewed by sspitsyn (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v9]
On Thu, 15 Apr 2021 17:02:22 GMT, Daniel D. Daugherty wrote: > Reviewed the v08 incremental. Still thumbs up. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v10]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request incrementally with one additional
commit since the last revision:
Fixed nits
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3191/files
- new: https://git.openjdk.java.net/jdk/pull/3191/files/27bf041c..d6e091da
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=09
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=08-09
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v9]
On Thu, 15 Apr 2021 22:22:53 GMT, David Holmes wrote:
> Only nits on the incremental.
>
> Like Dan I need to re-examine the complete set of changes to see if anything
> else sticks out (other than the non-encapsulation of the state changes around
> locking/unlocking when suspended).
>
> Thanks,
> David
Yes please.
Robbin
> src/hotspot/share/prims/jvmtiEnv.cpp line 952:
>
>> 950: if (!JvmtiSuspendControl::suspend(java_thread)) {
>> 951: // Either the thread is already suspended or
>> 952: // it was in process of exiting.
>
> Nit: the previous comment was perfectly correct. It is okay to replace the
> second "the thread" with "it" but it should still read "in _the_ process of
> exiting".
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 993:
>
>> 991: if (!JvmtiSuspendControl::suspend(java_thread)) {
>> 992: // Either the thread is already suspended or
>> 993: // it was in process of exiting.
>
> Ditto - "in the process ..."
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 1006:
>
>> 1004: if (!JvmtiSuspendControl::suspend(current)) {
>> 1005: // Either the thread is already suspended or
>> 1006: // it was in process of exiting.
>
> Ditto - "in the process ..."
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v9]
On Thu, 15 Apr 2021 07:14:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 13 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c
Only nits on the incremental.
Like Dan I need to re-examine the complete set of changes to see if anything
else sticks out (other than the non-encapsulation of the state changes around
locking/unlocking when suspended).
Thanks,
David
src/hotspot/share/prims/jvmtiEnv.cpp line 952:
> 950: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 951: // Either the thread is already suspended or
> 952: // it was in process of exiting.
Nit: the previous comment was perfectly correct. It is okay to replace the
second "the thread" with "it" but it should still read "in _the_ process of
exiting".
src/hotspot/share/prims/jvmtiEnv.cpp line 993:
> 991: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 992: // Either the thread is already suspended or
> 993: // it was in process of exiting.
Ditto - "in the process ..."
src/hotspot/share/prims/jvmtiEnv.cpp line 1006:
> 10
Re: RFR: 8257831: Suspend with handshakes [v9]
On Thu, 15 Apr 2021 07:14:09 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 13 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c
Reviewed the v08 incremental. Still thumbs up.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 16:30:56 GMT, Daniel D. Daugherty
wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed flag undef dep + spelling error
>
> src/hotspot/share/runtime/objectMonitor.cpp line 972:
>
>> 970:
>> 971: current->frame_anchor()->make_walkable(current);
>> 972: OrderAccess::storestore();
>
> Repeating this comment since my original is marked resolved, but I'm not
> seeing
> a new comment here. I originally pointed this out in the resolved
> conversation,
> but that doesn't make the comment show up in the review emails.
>
> Needs a comment explaining what the memory sync is for.
I seem to have fixed 2 of out 3.
> src/hotspot/share/runtime/objectMonitor.cpp line 1559:
>
>> 1557: if (_succ == current) {
>> 1558: _succ = NULL;
>> 1559: OrderAccess::fence();
>
> My original comment is marked "fixed", but I don't see the new comment:
>
> Please add a comment to this line:
> OrderAccess::fence(); // always do a full fence when successor is cleared
I fixed the one at line 982, it was not clear there were two from your comment
:) Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v9]
On Wed, 14 Apr 2021 16:27:10 GMT, Daniel D. Daugherty wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 13 commits: >> >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 4 >> - Fixed flag undef dep + spelling error >> - Obsolete unused flags >> - Review fixes 3 >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 2 >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - ... and 3 more: >> https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c > > src/hotspot/share/runtime/handshake.cpp line 415: > >> 413: // Adds are done lock free and so is arming. >> 414: // Calling this method with lock held is considered an error. >> 415: assert(!_lock.owned_by_self(), "Lock should not be held"); > > I originally added this comment inside the "resolved" conversation and that > kept > this comment from showing up. Let's try it as a new comment. > > Doesn't that mean the comment on L415 needs updating? > Should it be something like: > > // Synchronous adds are done lock free and so is arming, but some > // asynchronous adds are done when we already hold the lock. > > I'm not sure about the "some asynchronous adds" part... > @dcubed-ojdk The mutex is unrelated to adds/inserts. Adds/inserts to the queue can always be done regardless of which locks a thread may or may not have. The reason for not allowing inserts while holding the HandshakeState lock was to eliminate that from the table, since it have other implications. As @pchilano found we needed to reverse the order in should_process() to make it work (which I missed). So now when we have looked at the case, it is okay to do it. Meaning that you can add a handshake to yourself from another handshake regardless of it is sync or async. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 23:11:56 GMT, Serguei Spitsyn wrote:
> It looks good. I do not see any serviceability related related issues but
> posted some nits.
> Thanks,
> Serguei
Thank you Serguei!
> src/hotspot/share/prims/jvmtiEnv.cpp line 952:
>
>> 950: if (!JvmtiSuspendControl::suspend(java_thread)) {
>> 951: // Either the thread is already suspended or
>> 952: // the thread was in the process of exiting:
>
> Nit: replace this line with: "// it was in process of exiting."
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 993:
>
>> 991: if (!JvmtiSuspendControl::suspend(java_thread)) {
>> 992: // Either the thread is already suspended or
>> 993: // the thread was in the process of exiting:
>
> Nit: replace this line with: "// it was in process of exiting."
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 998:
>
>> 996: continue;
>> 997: }
>> 998: results[i] = JVMTI_ERROR_THREAD_SUSPENDED;
>
> Nit: Remove one extra space after '='.
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 1006:
>
>> 1004: if (!JvmtiSuspendControl::suspend(current)) {
>> 1005: // Either the thread is already suspended or
>> 1006: // the thread was in the process of exiting:
>
> Nit: replace this line with: "// it was in process of exiting."
Fixed
> src/hotspot/share/prims/jvmtiEnv.cpp line 1010:
>
>> 1008: results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;
>> 1009: } else {
>> 1010: results[self_index] = JVMTI_ERROR_THREAD_SUSPENDED;
>
> Nit: Remove one extra space after '='.
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v9]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 13 commits:
- Merge branch 'master' into SuspendInHandshake
- Review fixes 4
- Fixed flag undef dep + spelling error
- Obsolete unused flags
- Review fixes 3
- Merge branch 'master' into SuspendInHandshake
- Review fixes 2
- White space fixes
- Merge branch 'master' into SuspendInHandshake
- Review fixes
- ... and 3 more: https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c
-
Changes: https://git.openjdk.java.net/jdk/pull/3191/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=08
Stats: 1351 lines in 40 files changed: 273 ins; 882 del; 196 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 16:05:39 GMT, Daniel D. Daugherty
wrote:
>> Robbin Ehn has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed flag undef dep + spelling error
>
> src/hotspot/share/runtime/handshake.hpp line 99:
>
>> 97: // but we need to check for a safepoint before.
>> 98: // (this is due to a suspension handshake which put the JavaThread in
>> blocked
>> 99: // state so a safepoint may be in-progress)
>
> nit: s/(this/(This/
> nit: s/in-progress)/in-progress.)/
Fixed
> src/hotspot/share/runtime/handshake.hpp line 166:
>
>> 164: // thread gets suspended again (after a resume)
>> 165: // and we have not yet processed it.
>> 166: bool _async_suspend_handshake;
>
> Does this need to be `volatile`?
No, _async_suspend_handshake is now only loaded/stored with mutex held.
> src/hotspot/share/runtime/handshake.hpp line 177:
>
>> 175: void set_suspended(bool to) { return
>> Atomic::store(&_suspended, to); }
>> 176: bool has_async_suspend_handshake(){ return
>> _async_suspend_handshake; }
>> 177: void set_async_suspend_handshake(bool to) { _async_suspend_handshake
>> = to; }
>
> Does this need to be an Atomic::store?
No, _async_suspend_handshake is now only loaded/stored with mutex held.
> src/hotspot/share/runtime/objectMonitor.cpp line 424:
>
>> 422: // thread that suspended us.
>> 423: _recursions = 0;
>> 424: _succ = NULL;
>
> You might want to add a comment here:
> // Don't need a full fence after clearing successor here because of the call
> to exit().
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Fri, 9 Apr 2021 15:39:08 GMT, Daniel D. Daugherty wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains six commits: >> >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - Merge branch 'master' into SuspendInHandshake >> - Merge branch 'master' into SuspendInHandshake >> - 8257831: Suspend with handshake (review baseline) > > src/hotspot/share/runtime/objectMonitor.cpp line 410: > >> 408: >> 409: current->frame_anchor()->make_walkable(current); >> 410: OrderAccess::storestore(); > > Needs a comment explaining what the memory sync is for. Missed one, fixed. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On 15/04/2021 2:22 am, Daniel D.Daugherty wrote: On Wed, 7 Apr 2021 07:23:26 GMT, Robbin Ehn wrote: src/hotspot/share/runtime/handshake.cpp line 415: 413: // Adds are done lock free and so is arming. 414: // Calling this method with lock held is considered an error. 415: assert(!_lock.owned_by_self(), "Lock should not be held"); If adds are still lock-free then why was this assertion removed? Because we add the async handshake inside the sync handshake and thus we already hold the _lock. @robehn - You still haven't addressed this one... Assuming your comment is attributed to the correct comments, this is addressed. The assert has to be removed because we can own the lock. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 06:48:40 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed flag undef dep + spelling error
It looks good. I do not see any serviceability related related issues but
posted some nits.
Thanks,
Serguei
-
Marked as reviewed by sspitsyn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 06:48:40 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed flag undef dep + spelling error
src/hotspot/share/prims/jvmtiEnv.cpp line 952:
> 950: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 951: // Either the thread is already suspended or
> 952: // the thread was in the process of exiting:
Nit: replace this line with: "// it was in process of exiting."
src/hotspot/share/prims/jvmtiEnv.cpp line 993:
> 991: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 992: // Either the thread is already suspended or
> 993: // the thread was in the process of exiting:
Nit: replace this line with: "// it was in process of exiting."
src/hotspot/share/prims/jvmtiEnv.cpp line 998:
> 996: continue;
> 997: }
> 998: results[i] = JVMTI_ERROR_THREAD_SUSPENDED;
Nit: Remove one extra space after '='.
src/hotspot/share/prims/jvmtiEnv.cpp line 1006:
> 1004: if (!JvmtiSuspendControl::suspend(current)) {
> 1005: // Either the thread is already suspended or
> 1006: // the thread was in the process of exiting:
Nit: replace this line with: "// it was in process of exiting."
src/hotspot/share/prims/jvmtiEnv.cpp line 1010:
> 1008: results[self_index] = JVMTI_ERROR_THREAD_NOT_ALIVE;
> 1009: } else {
> 1010: results[self_index] = JVMTI_ERROR_THREAD_SUSPENDED;
Nit: Remove one extra space after '='.
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 06:48:40 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed flag undef dep + spelling error
I dug out the comments that I made in "resolved" sections where
the original comment wasn't actually resolved. I posted new comments
so let's see if those show up in the emails...
src/hotspot/share/runtime/handshake.cpp line 415:
> 413: // Adds are done lock free and so is arming.
> 414: // Calling this method with lock held is considered an error.
> 415: assert(!_lock.owned_by_self(), "Lock should not be held");
I originally added this comment inside the "resolved" conversation and that kept
this comment from showing up. Let's try it as a new comment.
Doesn't that mean the comment on L415 needs updating?
Should it be something like:
// Synchronous adds are done lock free and so is arming, but some
// asynchronous adds are done when we already hold the lock.
I'm not sure about the "some asynchronous adds" part...
@dcubed-ojdk
src/hotspot/share/runtime/objectMonitor.cpp line 972:
> 970:
> 971: current->frame_anchor()->make_walkable(current);
> 972: OrderAccess::storestore();
Repeating this comment since my original is marked resolved, but I'm not seeing
a new comment here. I originally pointed this out in the resolved conversation,
but that doesn't make the comment show up in the review emails.
Needs a comment explaining what the memory sync is for.
src
Re: RFR: 8257831: Suspend with handshakes [v4]
On Mon, 12 Apr 2021 07:48:10 GMT, Robbin Ehn wrote: >> src/hotspot/share/runtime/objectMonitor.cpp line 970: >> >>> 968: >>> 969: current->frame_anchor()->make_walkable(current); >>> 970: OrderAccess::storestore(); >> >> Needs a comment explaining what the memory sync is for. > > Fixed I'm not seeing the comment here. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 7 Apr 2021 07:23:26 GMT, Robbin Ehn wrote: >> src/hotspot/share/runtime/handshake.cpp line 415: >> >>> 413: // Adds are done lock free and so is arming. >>> 414: // Calling this method with lock held is considered an error. >>> 415: assert(!_lock.owned_by_self(), "Lock should not be held"); >> >> If adds are still lock-free then why was this assertion removed? > > Because we add the async handshake inside the sync handshake and thus we > already hold the _lock. @robehn - You still haven't addressed this one... - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 06:48:40 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed flag undef dep + spelling error
Reviewed v07 incremental. Still looks good.
Update: Did a crawl thru review of v07 full from the bottom up. I'm hoping
the change in review order results in less "change blindness" since I've
reviewed so many times. There are a few left overs that still need to be
address. There are a couple buried in "resolved comments" where it looks
like the issue that I raised is not actually resolved.
src/hotspot/share/runtime/handshake.hpp line 99:
> 97: // but we need to check for a safepoint before.
> 98: // (this is due to a suspension handshake which put the JavaThread in
> blocked
> 99: // state so a safepoint may be in-progress)
nit: s/(this/(This/
nit: s/in-progress)/in-progress.)/
src/hotspot/share/runtime/handshake.hpp line 166:
> 164: // thread gets suspended again (after a resume)
> 165: // and we have not yet processed it.
> 166: bool _async_suspend_handshake;
Does this need to be `volatile`?
src/hotspot/share/runtime/handshake.hpp line 177:
> 175: void set_suspended(bool to) { return
> Atomic::store(&_suspended, to); }
> 176: bool has_async_suspend_handshake(){ return
> _async_suspend_handshake; }
> 177: void set_async_suspend_handshake(bool to) { _async_suspend_handshake =
> to; }
Does this need to be an Atomic::store?
src/h
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 15:31:21 GMT, Daniel D. Daugherty wrote: > Reviewed v07 incremental. Still looks good. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
On Wed, 14 Apr 2021 06:48:40 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed flag undef dep + spelling error
Reviewed v07 incremental. Still looks good.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Wed, 14 Apr 2021 02:15:07 GMT, David Holmes wrote: > Latest updates look good - thanks. > > David Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v8]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request incrementally with one additional
commit since the last revision:
Fixed flag undef dep + spelling error
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3191/files
- new: https://git.openjdk.java.net/jdk/pull/3191/files/5034e8a5..451cdd00
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=07
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=06-07
Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Tue, 13 Apr 2021 16:49:53 GMT, Daniel D. Daugherty
wrote:
>> Robbin Ehn has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Obsolete unused flags
>> - Review fixes 3
>
> src/hotspot/share/runtime/arguments.cpp line 525:
>
>> 523: { "FlightRecorder", JDK_Version::jdk(13),
>> JDK_Version::undefined(), JDK_Version::undefined() },
>> 524: { "SuspendRetryCount",JDK_Version::jdk(16),
>> JDK_Version::jdk(17), JDK_Version::jdk(18) },
>> 525: { "SuspendRetryDelay",JDK_Version::jdk(16),
>> JDK_Version::jdk(17), JDK_Version::jdk(18) },
>
> I think both 'jdk(16)' values here need to be 'jdk(17)' since
> we didn't deprecate these option in JDK16. Or they might
> need to be 'JDK_Version::undefined()' since we didn't
> deprecate these options before obsoleting them.
> @dholmes-ora will know for sure.
Fixed with undefined() as David said.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Wed, 14 Apr 2021 02:10:18 GMT, David Holmes wrote: >> Robbin Ehn has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Obsolete unused flags >> - Review fixes 3 > > src/hotspot/share/runtime/thread.cpp line 1446: > >> 1444: // The careful dance between thread suspension and exit is handled >> here. >> 1445: // Since we are in thread_in_vm state and suspension is done with >> handshakes, >> 1446: // we can just put in the exiting state and it will be corrcetly >> handled. > > typo: corrcetly Fixed - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Tue, 13 Apr 2021 11:57:36 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Obsolete unused flags
> - Review fixes 3
Latest updates look good - thanks.
David
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Tue, 13 Apr 2021 11:57:36 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Obsolete unused flags
> - Review fixes 3
src/hotspot/share/runtime/thread.cpp line 1446:
> 1444: // The careful dance between thread suspension and exit is handled
> here.
> 1445: // Since we are in thread_in_vm state and suspension is done with
> handshakes,
> 1446: // we can just put in the exiting state and it will be corrcetly
> handled.
typo: corrcetly
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On 14/04/2021 2:59 am, Daniel D.Daugherty wrote:
On Tue, 13 Apr 2021 11:57:36 GMT, Robbin Ehn wrote:
Robbin Ehn has updated the pull request incrementally with two additional
commits since the last revision:
- Obsolete unused flags
- Review fixes 3
Still thumbs up.
I only looked at the v06 incremental webrev.
src/hotspot/share/runtime/arguments.cpp line 525:
523: { "FlightRecorder", JDK_Version::jdk(13),
JDK_Version::undefined(), JDK_Version::undefined() },
524: { "SuspendRetryCount",JDK_Version::jdk(16),
JDK_Version::jdk(17), JDK_Version::jdk(18) },
525: { "SuspendRetryDelay",JDK_Version::jdk(16),
JDK_Version::jdk(17), JDK_Version::jdk(18) },
I think both 'jdk(16)' values here need to be 'jdk(17)' since
we didn't deprecate these option in JDK16. Or they might
need to be 'JDK_Version::undefined()' since we didn't
deprecate these options before obsoleting them.
@dholmes-ora will know for sure.
It should be set to undefined() as they were never deprecated.
Thanks,
David
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Tue, 13 Apr 2021 11:57:36 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Obsolete unused flags
> - Review fixes 3
Still thumbs up.
I only looked at the v06 incremental webrev.
src/hotspot/share/runtime/arguments.cpp line 525:
> 523: { "FlightRecorder", JDK_Version::jdk(13),
> JDK_Version::undefined(), JDK_Version::undefined() },
> 524: { "SuspendRetryCount",JDK_Version::jdk(16),
> JDK_Version::jdk(17), JDK_Version::jdk(18) },
> 525: { "SuspendRetryDelay",JDK_Version::jdk(16),
> JDK_Version::jdk(17), JDK_Version::jdk(18) },
I think both 'jdk(16)' values here need to be 'jdk(17)' since
we didn't deprecate these option in JDK16. Or they might
need to be 'JDK_Version::undefined()' since we didn't
deprecate these options before obsoleting them.
@dholmes-ora will know for sure.
-
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
On Tue, 13 Apr 2021 11:57:36 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Obsolete unused flags
> - Review fixes 3
Update from reviews and pushed flag obsoleting.
CSR: https://bugs.openjdk.java.net/browse/JDK-8265124
Running test :)
@dholmes-ora the changes to set_terminated is not exactly rollback.
I notice the enum was private but set_terminated was public, so I changed the
enum to public and only added set_terminated.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v7]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request incrementally with two additional
commits since the last revision:
- Obsolete unused flags
- Review fixes 3
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3191/files
- new: https://git.openjdk.java.net/jdk/pull/3191/files/6bd645c5..5034e8a5
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=05-06
Stats: 50 lines in 9 files changed: 6 ins; 28 del; 16 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes
On Tue, 13 Apr 2021 07:44:58 GMT, David Holmes wrote: > Could be a neat use of a lambda "on_suspension_do" ... If no one else picks that up, I'll give it go after this :) - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On 13/04/2021 5:26 pm, Robbin Ehn wrote: On Tue, 13 Apr 2021 02:41:41 GMT, David Holmes wrote: Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'master' into SuspendInHandshake - Review fixes 2 - White space fixes - Merge branch 'master' into SuspendInHandshake - Review fixes - Merge branch 'master' into SuspendInHandshake - Merge branch 'master' into SuspendInHandshake - 8257831: Suspend with handshake (review baseline) src/hotspot/share/runtime/objectMonitor.cpp line 447: 445: // Completed the tranisition. 446: SafepointMechanism::process_if_requested(current); 447: current->set_thread_state(_thread_in_vm); I feel very uncomfortable that we remain _thread_blocked_trans across such a lengthy chunk of code - particularly the call to exit()! This is an abuse of the trans states which are only supposed to exist and be used to ensure the correctness of the Dekker-duality when setting and reading the thread state. And I still would prefer to see these state changes and related safepoint-mechanism checks encapsulated somehow. Yes we should figure out something here. Could be a neat use of a lambda "on_suspension_do" ... Note that we use to call exit() while blocked. Yes, it is the use of trans that concerns me here as very little code expects to encounter a trans-state. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Tue, 13 Apr 2021 02:51:32 GMT, David Holmes wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains eight commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - Review fixes 2
>> - White space fixes
>> - Merge branch 'master' into SuspendInHandshake
>> - Review fixes
>> - Merge branch 'master' into SuspendInHandshake
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/thread.hpp line 746:
>
>> 744:
>> 745: // _ParkEvent is cleared
>> 746: bool has_terminated() { return
>> Atomic::load(&_ParkEvent) == NULL; };
>
> Please change the comment to:
>
> // Termination indicator used by the signal handler. _ParkEvent is just a
> convenient field
> // we can NULL out after setting the JavaThread termination state (which
> can't itself be read
> // from the signal handler if a signal hits during the Thread destructor.
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Tue, 13 Apr 2021 02:46:46 GMT, David Holmes wrote: >> src/hotspot/share/runtime/thread.cpp line 455: >> >>> 453: ParkEvent::Release(_ParkEvent); >>> 454: // Can be racingly loaded in signal handler via has_terminated() >>> 455: Atomic::store(&_ParkEvent, (ParkEvent*)NULL); >> >> Nice solution for that odd signal handler issue. > > Yes nice simple fix, but note that the comments at lines 451 and 452 are > wrong - we can't ever encounter a null _ParkEvent, and we are not nulling for > good hygiene any more. I suggest the comment at line 454 be changed to read: > // Set to NULL as a termination indicator for has_terminated(). Fixed - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Tue, 13 Apr 2021 02:24:45 GMT, David Holmes wrote: >> Any unsafe (not native/blocked) is fine, therefore I never completed the >> transition. >> I set the state to _thread_in_vm before. > > Sorry I was misremembering the restriction, please remove the transition to > _thread_in_vm again (it messes up the comments at lines 436 and 437.) Fixed - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Tue, 13 Apr 2021 02:41:41 GMT, David Holmes wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains eight commits: >> >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 2 >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - Merge branch 'master' into SuspendInHandshake >> - Merge branch 'master' into SuspendInHandshake >> - 8257831: Suspend with handshake (review baseline) > > src/hotspot/share/runtime/objectMonitor.cpp line 447: > >> 445: // Completed the tranisition. >> 446: SafepointMechanism::process_if_requested(current); >> 447: current->set_thread_state(_thread_in_vm); > > I feel very uncomfortable that we remain _thread_blocked_trans across such a > lengthy chunk of code - particularly the call to exit()! This is an abuse of > the trans states which are only supposed to exist and be used to ensure the > correctness of the Dekker-duality when setting and reading the thread state. > > And I still would prefer to see these state changes and related > safepoint-mechanism checks encapsulated somehow. Yes we should figure out something here. Note that we use to call exit() while blocked. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Mon, 12 Apr 2021 20:51:17 GMT, Daniel D. Daugherty wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains eight commits: >> >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes 2 >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - Merge branch 'master' into SuspendInHandshake >> - Merge branch 'master' into SuspendInHandshake >> - 8257831: Suspend with handshake (review baseline) > > src/hotspot/share/runtime/handshake.cpp line 466: > >> 464: // The exception to this rule is the asynchronous suspension >> handshake. >> 465: // It by-passes the NSV by manully doing the transition. >> 466: // Since we can have safepoints while suspeneded we need to release >> the tty locker if it is held. > > nit typo: s/manully/manually/ > nit typo: s/suspeneded/suspended/ Fixed > src/hotspot/share/runtime/handshake.hpp line 133: > >> 131: // check the _lock, if held go to slow path. >> 132: // Since the handshakee is unsafe if _lock gets lock after this >> check >> 133: // we know another threads cannot process any handshakes. > > nit typo: s/_lock gets lock/_lock gets locked/ > nit typo: s/threads/thread/ Fixed > src/hotspot/share/runtime/thread.cpp line 1448: > >> 1446: // The careful dance between thread suspension and exit is handled >> here. >> 1447: // Since we are in vm and suspension is done with handshakes, >> 1448: // we can just put in the exiting state and it will be corrcetly >> handled. > > Perhaps: s/Since we are in vm and/Since we are in thread_in_vm state and/ > nit typo: s/corrcetly/correctly/ Fixed - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Thu, 8 Apr 2021 08:12:11 GMT, Serguei Spitsyn wrote: > The functions JvmtiSuspendControl::suspend and JvmtiSuspendControl::resume > are not really needed anymore. You probably decided to keep them around to > simplify merges a little bit. Is that right? There is also the debug printing method, which I did not know were to place. - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Mon, 12 Apr 2021 10:53:46 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
src/hotspot/share/runtime/thread.hpp line 746:
> 744:
> 745: // _ParkEvent is cleared
> 746: bool has_terminated() { return
> Atomic::load(&_ParkEvent) == NULL; };
Please change the comment to:
// Termination indicator used by the signal handler. _ParkEvent is just a
convenient field
// we can NULL out after setting the JavaThread termination state (which can't
itself be read
// from the signal handler if a signal hits during the Thread destructor.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Fri, 9 Apr 2021 15:45:50 GMT, Daniel D. Daugherty wrote: >> Robbin Ehn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains six commits: >> >> - White space fixes >> - Merge branch 'master' into SuspendInHandshake >> - Review fixes >> - Merge branch 'master' into SuspendInHandshake >> - Merge branch 'master' into SuspendInHandshake >> - 8257831: Suspend with handshake (review baseline) > > src/hotspot/share/runtime/thread.cpp line 455: > >> 453: ParkEvent::Release(_ParkEvent); >> 454: // Can be racingly loaded in signal handler via has_terminated() >> 455: Atomic::store(&_ParkEvent, (ParkEvent*)NULL); > > Nice solution for that odd signal handler issue. Yes nice simple fix, but note that the comments at lines 451 and 452 are wrong - we can't ever encounter a null _ParkEvent, and we are not nulling for good hygiene any more. I suggest the comment at line 454 be changed to read: // Set to NULL as a termination indicator for has_terminated(). - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Mon, 12 Apr 2021 10:53:46 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
src/hotspot/share/runtime/objectMonitor.cpp line 447:
> 445: // Completed the tranisition.
> 446: SafepointMechanism::process_if_requested(current);
> 447: current->set_thread_state(_thread_in_vm);
I feel very uncomfortable that we remain _thread_blocked_trans across such a
lengthy chunk of code - particularly the call to exit()! This is an abuse of
the trans states which are only supposed to exist and be used to ensure the
correctness of the Dekker-duality when setting and reading the thread state.
And I still would prefer to see these state changes and related
safepoint-mechanism checks encapsulated somehow.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 7 Apr 2021 07:20:15 GMT, Robbin Ehn wrote:
>> src/hotspot/share/prims/jvmtiRawMonitor.cpp line 428:
>>
>>> 426: jt->set_thread_state_fence(_thread_in_native_trans);
>>> 427: SafepointMechanism::process_if_requested(jt);
>>> 428: if (jt->is_interrupted(true)) {
>>
>> A thread must be _thread_in_vm to safely query is_interrupted() as it
>> accesses the threadObj.
>
> Any unsafe (not native/blocked) is fine, therefore I never completed the
> transition.
> I set the state to _thread_in_vm before.
Sorry I was misremembering the restriction, please remove the transition to
_thread_in_vm again (it messes up the comments at lines 436 and 437.)
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Mon, 12 Apr 2021 10:53:46 GMT, Robbin Ehn wrote:
>> 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:
>> https://github.com/openjdk/jdk/pull/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...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - Merge branch 'master' into SuspendInHandshake
> - Merge branch 'master' into SuspendInHandshake
> - 8257831: Suspend with handshake (review baseline)
I only found nits so still looks good to me.
src/hotspot/share/runtime/handshake.cpp line 466:
> 464: // The exception to this rule is the asynchronous suspension
> handshake.
> 465: // It by-passes the NSV by manully doing the transition.
> 466: // Since we can have safepoints while suspeneded we need to release
> the tty locker if it is held.
nit typo: s/manully/manually/
nit typo: s/suspeneded/suspended/
src/hotspot/share/runtime/handshake.hpp line 133:
> 131: // check the _lock, if held go to slow path.
> 132: // Since the handshakee is unsafe if _lock gets lock after this check
> 133: // we know another threads cannot process any handshakes.
nit typo: s/_lock gets lock/_lock gets locked/
nit typo: s/threads/thread/
src/hotspot/share/runtime/thread.cpp line 1448:
> 1446: // The careful dance between thread suspension and exit is handled
> here.
> 1447: // Since we are in vm and suspension is done with handshakes,
> 1448: // we ca
Re: RFR: 8257831: Suspend with handshakes [v6]
On 12/04/2021 8:53 pm, Robbin Ehn wrote:
On Wed, 7 Apr 2021 13:57:50 GMT, Robbin Ehn wrote:
src/hotspot/share/runtime/thread.inline.hpp line 207:
205: }
206:
207: inline void JavaThread::set_terminated(TerminatedTypes t) {
I prefer set_terminated(arg) over the new set of methods.
We had two methods:
void set_terminated(TerminatedTypes t);
void set_terminated_value();
Terminated is part of the name of the method, the name of the flag, the name of
the type and part of the names of two of the states, which is very confusing.
I'll admit the API is not that clean but I would expect the method, the
flag, the type, to all share a common name as they are all related to
the same underlying thing: the termination state.
set_terminated_value() seems completely unnecessary as a special-case.
Also the setters now match the queries:
E.g.
`bool is_exiting()`
The queries do not indicate in any sense that they are queries on the
terminated flag.
The state flag is an implementation detail from query POV.
So to be consistent e.g. "set_exiting()" also hides the fact that we keep track
of this with a flag.
The point of the flag is that it is tracking a singular termination
state with a progression from _not_terminated, through _thread_exiting,
to _thread terminated, or the special state _vm_terminated (though I'm
not sure why we need that ...)
Please advise :) , I can roll back if you insist!
There is always room for improvement with these things, but the changes
you are making to this part of the thread API seem completely unrelated
to thread suspension, so I'd ask that you please roll them back, for now
at least.
Thanks,
David
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Mon, 12 Apr 2021 07:57:14 GMT, Robbin Ehn wrote: > I'm still really happy with this fix! Thumbs up! > Reminder: don't forget to decide what to do about the obsolete options. > @dholmes-ora can provide guidance on how to handle this type of removal. Thanks, yes I'll start a CSR! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
On Wed, 7 Apr 2021 13:57:50 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/thread.inline.hpp line 207:
>>
>>> 205: }
>>> 206:
>>> 207: inline void JavaThread::set_terminated(TerminatedTypes t) {
>>
>> I prefer set_terminated(arg) over the new set of methods.
>
> We had two methods:
>
>void set_terminated(TerminatedTypes t);
>void set_terminated_value();
>
> Terminated is part of the name of the method, the name of the flag, the name
> of the type and part of the names of two of the states, which is very
> confusing.
>
> Also the setters now match the queries:
> E.g.
> `bool is_exiting()`
>
> The queries do not indicate in any sense that they are queries on the
> terminated flag.
> The state flag is an implementation detail from query POV.
> So to be consistent e.g. "set_exiting()" also hides the fact that we keep
> track of this with a flag.
Please advise :) , I can roll back if you insist!
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v6]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains eight commits:
- Merge branch 'master' into SuspendInHandshake
- Review fixes 2
- White space fixes
- Merge branch 'master' into SuspendInHandshake
- Review fixes
- Merge branch 'master' into SuspendInHandshake
- Merge branch 'master' into SuspendInHandshake
- 8257831: Suspend with handshake (review baseline)
-
Changes: https://git.openjdk.java.net/jdk/pull/3191/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=05
Stats: 1332 lines in 39 files changed: 273 ins; 863 del; 196 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v5]
> 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:
> https://github.com/openjdk/jdk/pull/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...
Robbin Ehn has updated the pull request incrementally with one additional
commit since the last revision:
Review fixes 2
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/3191/files
- new: https://git.openjdk.java.net/jdk/pull/3191/files/ed8accf0..7e704326
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3191&range=03-04
Stats: 51 lines in 4 files changed: 18 ins; 24 del; 9 mod
Patch: https://git.openjdk.java.net/jdk/pull/3191.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3191/head:pull/3191
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v2]
On Wed, 7 Apr 2021 12:57:01 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.cpp line 677:
>>
>>> 675: } else {
>>> 676: // Target is going to wake up and leave suspension.
>>> 677: // Let's just stop the thread from doing that.
>>
>> IIUC this would be the case where the target was hit with a suspend request
>> but has not yet processed the actual suspension handshake op, then a resume
>> comes in so suspended is no longer true, and then another suspend request is
>> made (this one) which simply turns the suspended flag back on - is that
>> right?
>> Overall I'm finding it very hard to see what the two suspend state flags
>> actually signify. I'd like to see that written up somewhere.
>
> Sure
I pushed some changes with some more comments and renamed the flag.
I hope that help, if not, let me know.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Mon, 12 Apr 2021 07:56:36 GMT, Richard Reingruber wrote: > Nice > Richard. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Thu, 1 Apr 2021 03:24:45 GMT, Patricio Chilano Mateo
wrote:
>> src/hotspot/share/runtime/thread.cpp line 667:
>>
>>> 665:
>>> 666: // We wait for the thread to transition to a more usable state.
>>> 667: for (int i = 1; i <= SuspendRetryCount; i++) {
>>
>> You will need to obsolete SuspendRetryCount and SuspendRetryDelay. This will
>> need a CSR request indicating why there is no deprecation step.
>
> I think we will need to do the same for flags AssertOnSuspendWaitFailure and
> TraceSuspendWaitFailures.
I'll create CSR for the related to this change-set flags, SuspendRetryCount and
SuspendRetryDelay.
-
PR: https://git.openjdk.java.net/jdk/pull/3191
Re: RFR: 8257831: Suspend with handshakes [v4]
On Mon, 12 Apr 2021 06:42:47 GMT, Robbin Ehn wrote: > > > Technically you are correct. > I have tested to remove this is previously version and all tests passes fine. > The reason I kept it is because the suspender have identified a thread for > suspension and deemed it suspendable, so we play nice. > To know why suspend failed the suspender must check if thread is exiting > after a failed suspend. (originally I had a bug here which caused me to > wrongfully introduce this from the beginning) Thanks for the explanation. As I understand you want to be fault tolerant towards somewhat sloppy agents. That's of course ok. It should be explained in a comment though. > I'll remove it, since it simplifies the code and David's comments about this > code is now out-of-line can be fixed. Also it does not eliminate the race as far as I can tell. - PR: https://git.openjdk.java.net/jdk/pull/3191
