Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @robehn - Thanks for the re-reviews! - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > coleenp CR - clarify need for ThreadsListHandle in > JvmtiEventControllerPrivate::recompute_enabled(). Marked as reviewed by rehn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > coleenp CR - clarify need for ThreadsListHandle in > JvmtiEventControllerPrivate::recompute_enabled(). Thanks Dan - updates look good. David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 21:54:01 GMT, Coleen Phillimore wrote: >> I've written up a rather long analysis about how the use of >> `JvmtiThreadState_lock` >> in `JvmtiEventControllerPrivate::recompute_enabled()` means that we can >> safely >> traverse the JvmtiThreadState list returned by `JvmtiThreadState::first()` >> without >> racing with an exiting JavaThread. I've sent it to you via direct email. >> >> I could amend the comment above the ThreadsListHandle like this: >> >> // If we have a JvmtiThreadState, then we've reached the point where >> // threads can exist so create a ThreadsListHandle to protect them. >> // The held JvmtiThreadState_lock prevents exiting JavaThreads from >> // being removed from the JvmtiThreadState list we're about to walk >> // so this ThreadsListHandle exists just to satisfy the lower level >> sanity >> // checks that the target JavaThreads are protected. >> ThreadsListHandle tlh; > > Yes, this comment is good and it does explain why it's safe and why the TLH > is there. Thanks for doing the deep dive and explanation. Updated comment has been pushed to this PR. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > coleenp CR - clarify need for ThreadsListHandle in > JvmtiEventControllerPrivate::recompute_enabled(). Updated comment has been pushed to this PR. For some reason, the comments that @coleenp and I are posting in the "Files changed" view are not showing up here. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v13]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: coleenp CR - clarify need for ThreadsListHandle in JvmtiEventControllerPrivate::recompute_enabled(). - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/91173506..fe28cbe9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=11-12 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 20:46:21 GMT, Daniel D. Daugherty wrote: >> JvmtiThreadState objects point to JavaThread and vice versa, so I still >> don't see why you don't protect the first element. > > I've written up a rather long analysis about how the use of > `JvmtiThreadState_lock` > in `JvmtiEventControllerPrivate::recompute_enabled()` means that we can safely > traverse the JvmtiThreadState list returned by `JvmtiThreadState::first()` > without > racing with an exiting JavaThread. I've sent it to you via direct email. > > I could amend the comment above the ThreadsListHandle like this: > > // If we have a JvmtiThreadState, then we've reached the point where > // threads can exist so create a ThreadsListHandle to protect them. > // The held JvmtiThreadState_lock prevents exiting JavaThreads from > // being removed from the JvmtiThreadState list we're about to walk > // so this ThreadsListHandle exists just to satisfy the lower level sanity > // checks that the target JavaThreads are protected. > ThreadsListHandle tlh; Yes, this comment is good and it does explain why it's safe and why the TLH is there. Thanks for doing the deep dive and explanation. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v12]
On Fri, 5 Nov 2021 15:43:37 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr4.patch > JvmtiThreadState objects point to JavaThread and vice versa, so I still don't > see why you don't protect the first element. I've written up a rather long analysis about how the use of JvmtiThreadState_lock in JvmtiEventControllerPrivate::recompute_enabled() means that we can safely traverse the JvmtiThreadState list returned by JvmtiThreadState::first() without racing with an exiting JavaThread. I've sent it to you via direct email. I could amend the comment above the ThreadsListHandle like this: // If we have a JvmtiThreadState, then we've reached the point where // threads can exist so create a ThreadsListHandle to protect them. // The held JvmtiThreadState_lock prevents exiting JavaThreads from // being removed from the JvmtiThreadState list we're about to walk // so this ThreadsListHandle exists just to satisfy the lower level sanity // checks that the target JavaThreads are protected. ThreadsListHandle tlh; - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 16:43:04 GMT, Coleen Phillimore wrote: >> The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState` >> objects. >> `JvmtiThreadState::first()` returns the head of the global list of >> `JvmtiThreadState` >> objects for the system. Each `JvmtiThreadState` object contains a >> `JavaThread*` and >> we have to protect use of the `JavaThread*` which can happen in the >> `recompute_thread_enabled(state)` call below. > > JvmtiThreadState objects point to JavaThread and vice versa, so I still don't > see why you don't protect the first element. I've written up a rather long analysis about how the use of `JvmtiThreadState_lock` in `JvmtiEventControllerPrivate::recompute_enabled()` means that we can safely traverse the JvmtiThreadState list returned by `JvmtiThreadState::first()` without racing with an exiting JavaThread. I've sent it to you via direct email. I could amend the comment above the ThreadsListHandle like this: // If we have a JvmtiThreadState, then we've reached the point where // threads can exist so create a ThreadsListHandle to protect them. // The held JvmtiThreadState_lock prevents exiting JavaThreads from // being removed from the JvmtiThreadState list we're about to walk // so this ThreadsListHandle exists just to satisfy the lower level sanity // checks that the target JavaThreads are protected. ThreadsListHandle tlh; - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v12]
On Fri, 5 Nov 2021 15:43:37 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr4.patch Thanks for the offline comments. I still think refactoring is_JavaThread_protected would be a lot nicer but that's my opinion and you don't have to change it. - Marked as reviewed by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @sspitsyn - I still need a re-review from you guys. @robehn - A re-review from you would be good, but I think I cleanly addressed your last comments... - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 5 Nov 2021 15:38:27 GMT, Daniel D. Daugherty wrote: >> Should the ThreadsListHandle protect JvmtiThreadState::first also? If >> state->next() needs it why doesn't the first entry need this? There's no >> atomic load on the _head field. > > The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState` > objects. > `JvmtiThreadState::first()` returns the head of the global list of > `JvmtiThreadState` > objects for the system. Each `JvmtiThreadState` object contains a > `JavaThread*` and > we have to protect use of the `JavaThread*` which can happen in the > `recompute_thread_enabled(state)` call below. JvmtiThreadState objects point to JavaThread and vice versa, so I still don't see why you don't protect the first element. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Mon, 1 Nov 2021 15:59:58 GMT, Daniel D. Daugherty wrote: >> Update: I've added comments to WB_HandshakeReadMonitors() and >> WB_HandshakeWalkStack() to clarify their expectations. > > Update again: I took a closer look at `WB_AsyncHandshakeWalkStack()`, > `WB_HandshakeReadMonitors()` and `WB_HandshakeWalkStack()` and > I realized that those functions were not properly converting a `jobject` into > a protected JavaThread*. I've updated them to call the proper conversion > function and I've changed this code to `guarantee()` that the target is not > `nullptr`. Great. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 21:45:16 GMT, Coleen Phillimore wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8249004.cr2.patch > > Sorry for such a long delay looking at this. I had a couple questions and a > suggestion. @coleenp - Thanks for the re-review! - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Fri, 5 Nov 2021 15:15:14 GMT, Daniel D. Daugherty wrote: >> Yes, ^ that might make the logic easier to follow. I can't figure out what >> checkTLSOnly means. Could it be refactored into a different function like >> check_TLS() and then call it in the place where you pass true instead of >> is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? > > I've changed `is_JavaThread_protected()` to materialize current_thread when > it is declared > to simplify the code paths. > I can't figure out what checkTLSOnly means. See the function's header comment: // ... If checkTLHOnly is true (default is false), then we only check // if the target JavaThread is protected by a ThreadsList (if any) associated // with the calling Thread. > Could it be refactored into a different function like check_TLS() and then > call it in the place where you pass true instead of is_JavaThread_protected? I had a copy of this logic in a separate function and @dholmes-ora suggested that I add the `checkTLHOnly` parameter to this function and get rid of the separate copy so I've been there and done that. I could *move* the logic into a separate function and then call that new function from here and from the other call sites that pass `true`, but then I'd have to materialize current_thread in that new function and then on this code path we would be materializing current_thread twice. I don't want to do that. > Does checkTLSOnly skip a lot of these checks? Please read the header comment to see what `checkTLHOnly` does. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Thu, 4 Nov 2021 19:33:33 GMT, Coleen Phillimore wrote: >> It wasn't quite missing from the baseline code. This version of execute(): >> >> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` >> >> used to always create a ThreadsListHandle. I added a `ThreadsListHandle*` >> parameter to that version and created a wrapper with the existing signature >> to pass `nullptr` to the execute() version with the `ThreadsListHandle*` >> parameter. What that means is that all existing callers of: >> >> `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` >> >> no longer had a ThreadsListHandle created for them. With the new sanity >> check in place, I shook the trees to make sure that we had explicit >> ThreadsListHandles in place for the locations that needed them. >> >> `JvmtiEventControllerPrivate::recompute_enabled()` happened to be >> one of the places where the ThreadsListHandle created by execute() >> was hiding the fact that `recompute_enabled()` needed one. > > Should the ThreadsListHandle protect JvmtiThreadState::first also? If > state->next() needs it why doesn't the first entry need this? There's no > atomic load on the _head field. The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState` objects. `JvmtiThreadState::first()` returns the head of the global list of `JvmtiThreadState` objects for the system. Each `JvmtiThreadState` object contains a `JavaThread*` and we have to protect use of the `JavaThread*` which can happen in the `recompute_thread_enabled(state)` call below. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 21:44:45 GMT, Coleen Phillimore wrote: >> I'm really tempted to go ahead and change it to always set >> current thread when it is declared and then clean things up a bit. > > Yes, ^ that might make the logic easier to follow. I can't figure out what > checkTLSOnly means. Could it be refactored into a different function like > check_TLS() and then call it in the place where you pass true instead of > is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? I've changed `is_JavaThread_protected()` to materialize current_thread when it is declared to simplify the code paths. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v12]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr4.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/86fcdfbe..91173506 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=10-11 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 17:02:59 GMT, Daniel D. Daugherty wrote: >> Sorry I missed that line 463 is still within the else from line 447. >> >> Thread::current() is a compiler-defined thread-local access so should be >> relatively cheap these days, but I have no numbers. > > I'm really tempted to go ahead and change it to always set > current thread when it is declared and then clean things up a bit. Yes, ^ that might make the logic easier to follow. I can't figure out what checkTLSOnly means. Could it be refactored into a different function like check_TLS() and then call it in the place where you pass true instead of is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks? - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 21:34:50 GMT, Daniel D. Daugherty wrote: >> src/hotspot/share/prims/jvmtiEventController.cpp line 623: >> >>> 621: // If we have a JvmtiThreadState, then we've reached the point >>> where >>> 622: // threads can exist so create a ThreadsListHandle to protect them. >>> 623: ThreadsListHandle tlh; >> >> Good catch on the missing TLH for this code. > > It wasn't quite missing from the baseline code. This version of execute(): > > `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` > > used to always create a ThreadsListHandle. I added a `ThreadsListHandle*` > parameter to that version and created a wrapper with the existing signature > to pass `nullptr` to the execute() version with the `ThreadsListHandle*` > parameter. What that means is that all existing callers of: > > `Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)` > > no longer had a ThreadsListHandle created for them. With the new sanity > check in place, I shook the trees to make sure that we had explicit > ThreadsListHandles in place for the locations that needed them. > > `JvmtiEventControllerPrivate::recompute_enabled()` happened to be > one of the places where the ThreadsListHandle created by execute() > was hiding the fact that `recompute_enabled()` needed one. Should the ThreadsListHandle protect JvmtiThreadState::first also? If state->next() needs it why doesn't the first entry need this? There's no atomic load on the _head field. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr2.patch Sorry for such a long delay looking at this. I had a couple questions and a suggestion. - Changes requested by coleenp (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:16:03 GMT, David Holmes wrote: >> I suspect that the way that git is displaying the diffs is confusing you. >> >> We need `current_thread` set if we get to line 474 so we have to init >> `current_thread` on line 446 for the `checkTLHOnly == true` case and >> on line 463 for the `checkTLHOnly == false` case. >> >> I could simplify the logic by always setting current thread when it is >> declared on 444, but I was trying to avoid the call to `Thread::current()` >> until I actually needed it. I thought `Thread::current()` can be expensive. >> Is this no longer the case? > > Sorry I missed that line 463 is still within the else from line 447. > > Thread::current() is a compiler-defined thread-local access so should be > relatively cheap these days, but I have no numbers. I'm really tempted to go ahead and change it to always set current thread when it is declared and then clean things up a bit. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Thu, 4 Nov 2021 01:18:23 GMT, David Holmes wrote:
>> The rationale for removing the is_exiting() check from `java_suspend()` was
>> that it
>> was redundant because the handshake code detected and handled the
>> `is_exiting()`
>> case so we didn't need to do that work twice.
>>
>> If we look at `HandshakeState::resume()` there is no logic for detecting or
>> handling
>> the possibility of an exiting thread. That being said, we have to look
>> closer at what
>> `HandshakeState::resume()` does and whether that logic can be harmful if
>> executed
>> on an exiting thread.
>>
>> Here's the code:
>>
>> bool HandshakeState::resume() {
>> if (!is_suspended()) {
>> return false;
>> }
>> MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
>> if (!is_suspended()) {
>> assert(!_handshakee->is_suspended(), "cannot be suspended without a
>> suspend request");
>> return false;
>> }
>> // Resume the thread.
>> set_suspended(false);
>> _lock.notify();
>> return true;
>> }
>>
>>
>> I'm not seeing anything in `HandshakeState::resume()` that
>> worries me with respect to an exiting thread. Of course, the
>> proof is in the testing so I'll rerun the usual testing after
>> deleting that code.
>
> A suspended thread cannot be exiting - else the suspend logic is broken. So,
> given you can call `resume()` on a not-suspended thread, as long as the
> handshake code checks for `is_supended()` (which it does) then no explicit
> `is_exiting` check is needed.
Agreed! I have to keep reminding myself that with handshake based suspend
and resume, we just don't have the same races with exiting threads that we
used to have.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora and @robehn - Thanks for your reviews on the previous version (8249004.cr2.patch). Mach5 Tier[1-5] testing has finished and looks good; Mach5 Tier[678] are still running. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v11]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr3.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/3e1d1b06..86fcdfbe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=09-10 Stats: 10 lines in 3 files changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 16:05:21 GMT, Daniel D. Daugherty wrote:
>> Yes, please
>
> The rationale for removing the is_exiting() check from `java_suspend()` was
> that it
> was redundant because the handshake code detected and handled the
> `is_exiting()`
> case so we didn't need to do that work twice.
>
> If we look at `HandshakeState::resume()` there is no logic for detecting or
> handling
> the possibility of an exiting thread. That being said, we have to look closer
> at what
> `HandshakeState::resume()` does and whether that logic can be harmful if
> executed
> on an exiting thread.
>
> Here's the code:
>
> bool HandshakeState::resume() {
> if (!is_suspended()) {
> return false;
> }
> MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
> if (!is_suspended()) {
> assert(!_handshakee->is_suspended(), "cannot be suspended without a
> suspend request");
> return false;
> }
> // Resume the thread.
> set_suspended(false);
> _lock.notify();
> return true;
> }
>
>
> I'm not seeing anything in `HandshakeState::resume()` that
> worries me with respect to an exiting thread. Of course, the
> proof is in the testing so I'll rerun the usual testing after
> deleting that code.
A suspended thread cannot be exiting - else the suspend logic is broken. So,
given you can call `resume()` on a not-suspended thread, as long as the
handshake code checks for `is_supended()` (which it does) then no explicit
`is_exiting` check is needed.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 15:45:06 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/share/runtime/thread.cpp line 446:
>>
>>> 444: Thread* current_thread = nullptr;
>>> 445: if (checkTLHOnly) {
>>> 446: current_thread = Thread::current();
>>
>> This seems redundant due to line 463. You can just have a `if
>> (!checkTLHOnly)` block here.
>
> I suspect that the way that git is displaying the diffs is confusing you.
>
> We need `current_thread` set if we get to line 474 so we have to init
> `current_thread` on line 446 for the `checkTLHOnly == true` case and
> on line 463 for the `checkTLHOnly == false` case.
>
> I could simplify the logic by always setting current thread when it is
> declared on 444, but I was trying to avoid the call to `Thread::current()`
> until I actually needed it. I thought `Thread::current()` can be expensive.
> Is this no longer the case?
Sorry I missed that line 463 is still within the else from line 447.
Thread::current() is a compiler-defined thread-local access so should be
relatively cheap these days, but I have no numbers.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 09:50:08 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.cpp line 350:
>>
>>> 348: }
>>> 349:
>>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>>> tlh_p, JavaThread* target) {
>>
>> Nit: can we drop the `_p` part of `tlh_p` please.
>
> Yes, please.
Fixed in handshake.[ch]pp.
>> src/hotspot/share/runtime/thread.cpp line 1764:
>>
>>> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
>>> true),
>>> 1763: "missing ThreadsListHandle in calling context.");
>>> 1764: if (is_exiting()) {
>>
>> Can't we remove this the same as we did for `java_suspend()`?
>
> Yes, please
The rationale for removing the is_exiting() check from `java_suspend()` was
that it
was redundant because the handshake code detected and handled the `is_exiting()`
case so we didn't need to do that work twice.
If we look at `HandshakeState::resume()` there is no logic for detecting or
handling
the possibility of an exiting thread. That being said, we have to look closer
at what
`HandshakeState::resume()` does and whether that logic can be harmful if
executed
on an exiting thread.
Here's the code:
bool HandshakeState::resume() {
if (!is_suspended()) {
return false;
}
MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
if (!is_suspended()) {
assert(!_handshakee->is_suspended(), "cannot be suspended without a suspend
request");
return false;
}
// Resume the thread.
set_suspended(false);
_lock.notify();
return true;
}
I'm not seeing anything in `HandshakeState::resume()` that
worries me with respect to an exiting thread. Of course, the
proof is in the testing so I'll rerun the usual testing after
deleting that code.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 01:21:50 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/thread.cpp line 446:
>
>> 444: Thread* current_thread = nullptr;
>> 445: if (checkTLHOnly) {
>> 446: current_thread = Thread::current();
>
> This seems redundant due to line 463. You can just have a `if
> (!checkTLHOnly)` block here.
I suspect that the way that git is displaying the diffs is confusing you.
We need `current_thread` set if we get to line 474 so we have to init
`current_thread` on line 446 for the `checkTLHOnly == true` case and
on line 463 for the `checkTLHOnly == false` case.
I could simplify the logic by always setting current thread when it is
declared on 444, but I was trying to avoid the call to `Thread::current()`
until I actually needed it. I thought `Thread::current()` can be expensive.
Is this no longer the case?
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Wed, 3 Nov 2021 01:19:21 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/handshake.cpp line 350:
>
>> 348: }
>> 349:
>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>> tlh_p, JavaThread* target) {
>
> Nit: can we drop the `_p` part of `tlh_p` please.
Yes, please.
> src/hotspot/share/runtime/thread.cpp line 1764:
>
>> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
>> true),
>> 1763: "missing ThreadsListHandle in calling context.");
>> 1764: if (is_exiting()) {
>
> Can't we remove this the same as we did for `java_suspend()`?
Yes, please
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Daniel D. Daugherty has updated the pull request incrementally with one > additional commit since the last revision: > > 8249004.cr2.patch Marked as reviewed by rehn (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote:
>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>>
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Daniel D. Daugherty has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8249004.cr2.patch
Hi Dan,
Generally seems okay but a couple of minor issues below.
Thanks,
David
src/hotspot/share/runtime/handshake.cpp line 350:
> 348: }
> 349:
> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
> tlh_p, JavaThread* target) {
Nit: can we drop the `_p` part of `tlh_p` please.
src/hotspot/share/runtime/thread.cpp line 446:
> 444: Thread* current_thread = nullptr;
> 445: if (checkTLHOnly) {
> 446: current_thread = Thread::current();
This seems redundant due to line 463. You can just have a `if (!checkTLHOnly)`
block here.
src/hotspot/share/runtime/thread.cpp line 1764:
> 1762: guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */
> true),
> 1763: "missing ThreadsListHandle in calling context.");
> 1764: if (is_exiting()) {
Can't we remove this the same as we did for `java_suspend()`?
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 18:31:26 GMT, Coleen Phillimore wrote: >> Daniel D. Daugherty has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8249004.cr1.patch > > This has more moving pieces than the last version. I'm a bit uneasy about > passing NULL as a thread to Handshake::execute(). This shouldn't be something > that should happen. @coleenp and @dholmes-ora, the latest version should address the last of your previous comments. Please re-review when you get the chance. @robehn and @sspitsyn - It would be good to get re-reviews from you guys on this latest version. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v10]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr2.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/045b3e0d..3e1d1b06 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=08-09 Stats: 128 lines in 5 files changed: 22 ins; 46 del; 60 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Mon, 1 Nov 2021 01:55:48 GMT, David Holmes wrote: >> U... The purpose of the new `is_exiting()` check and the baseline's >> `ThreadsListHandle::includes()` check is to avoid making this call: >> >> return this->handshake_state()->suspend(); >> >> The call we are avoiding is the one that makes the synchronous >> SuspendThreadHandshake request (for threads other than self) >> so by detecting the exiting thread early, we are avoiding entering >> the handshake machinery. > > But why do we care about that? The low-level mechanism handles exiting > threads so there is no need for the higher-level code to do that. The current > logic gives the appearance that we must filter out exiting threads at this > level because the lower-level code does not handle them. Okay you've convinced me. I'm merging my latest patch with master and then I'll retest with the is_exiting() check removed. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Sat, 16 Oct 2021 15:58:21 GMT, Daniel D. Daugherty
wrote:
>> The `NULL` target thread being passed in is actually handled by the baseline
>> code:
>>
>>
>> ThreadsListHandle tlh;
>> if (tlh.includes(target)) {
>>
>>
>> `tlh.includes(target)` returns `false` when `target` is `NULL/nullptr`.
>> I just made the already handled situation more explicit.
>>
>>> Why would the caller not check if the target is dead?
>>
>> Hmmm... It's hard for me to answer that question since I didn't write
>> the original code. The test code that calls `WB_HandshakeWalkStack()`
>> or `WB_AsyncHandshakeWalkStack()` can call those functions with
>> a `thread_handle` that translates into a `thread_oop` that returns a
>> `NULL` `JavaThread*`.
>>
>> See the comment that I added to `WB_AsyncHandshakeWalkStack()` above.
>
> Update: I've added comments to WB_HandshakeReadMonitors() and
> WB_HandshakeWalkStack() to clarify their expectations.
Update again: I took a closer look at `WB_AsyncHandshakeWalkStack()`,
`WB_HandshakeReadMonitors()` and `WB_HandshakeWalkStack()` and
I realized that those functions were not properly converting a `jobject` into
a protected JavaThread*. I've updated them to call the proper conversion
function and I've changed this code to `guarantee()` that the target is not
`nullptr`.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 29 Oct 2021 22:16:17 GMT, Daniel D. Daugherty
wrote:
>> While the name is somewhat ungainly - and unnecessarily detailed given
>> `is_JavaThread_protected` has a similar constraint - it should be a static
>> function as given because it must only be called on the current thread, and
>> an instance method would give the false impression that it could be called
>> on any thread.
>>
>> That said it should be possible to write that code block only once and reuse
>> it. And the name as I said is somewhat ungainly. You could even have:
>>
>>
>> static bool is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly =
>> false) {
>
> I'm checking out adding the `checkTLHOnly` param to
> `is_JavaThread_protected()`
> and replacing `is_JavaThread_protected_by_my_ThreadsList()` calls with calls
> to the updated function.
Testing went well so the above change will be in the next change that I push.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v9]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Merge branch 'master' into JDK-8249004 - coleenp CR - add comments to clarify what WB_HandshakeReadMonitors() and WB_HandshakeWalkStack() expect. - dholmes CR - change NULL to nullptr. - 8249004.cr1.patch - Merge branch 'master' into JDK-8249004 - Merge branch 'master' into JDK-8249004 - Merge branch 'master' into JDK-8249004 - Merge branch 'master' into JDK-8249004 - Merge branch 'master' into JDK-8249004 - 8249004: Reduce ThreadListHandle overhead in relation to direct handshakes - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/9518a9a8..045b3e0d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=07-08 Stats: 44007 lines in 1055 files changed: 33358 ins; 6622 del; 4027 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 29 Oct 2021 22:35:50 GMT, Daniel D. Daugherty wrote: >> The `is_exiting` check seems unnecessary as the handshake code will not >> handshake with an exiting thread. The nested TLH was unnecessary too AFAICS. > > U... The purpose of the new `is_exiting()` check and the baseline's > `ThreadsListHandle::includes()` check is to avoid making this call: > > return this->handshake_state()->suspend(); > > The call we are avoiding is the one that makes the synchronous > SuspendThreadHandshake request (for threads other than self) > so by detecting the exiting thread early, we are avoiding entering > the handshake machinery. But why do we care about that? The low-level mechanism handles exiting threads so there is no need for the higher-level code to do that. The current logic gives the appearance that we must filter out exiting threads at this level because the lower-level code does not handle them. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Sun, 17 Oct 2021 12:52:15 GMT, David Holmes wrote:
>> On rereading all of these comments and the current baseline code, I have
>> to clarify one thing:
>>
>> There is a minor change in behavior caused by switching from a
>> `ThreadsListHandle::includes()` check to a `JavaThread::is_exiting()`
>> check. The `JavaThread::is_exiting()` will return true before the target
>> JavaThread is removed from the system's current ThreadsList. So the
>> `JavaThread::is_exiting()` check will return true faster and the
>> `ThreadsListHandle::includes()` check.
>>
>> However, that change in behavior does not result in a change in
>> behavior for a suspend thread request. Here's the relevant code:
>>
>>
>> src/hotspot/share/runtime/thread.cpp:
>> void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
>>
>>
>> // The careful dance between thread suspension and exit is handled here.
>> // Since we are in thread_in_vm state and suspension is done with
>> handshakes,
>> // we can just put in the exiting state and it will be correctly handled.
>> set_terminated(_thread_exiting);
>>
>>
>>
>> // Remove from list of active threads list, and notify VM thread if we are
>> the last non-daemon thread
>> Threads::remove(this, daemon);
>>
>>
>> }
>>
>>
>> When we changed the suspend thread mechanism to use handshakes, we
>> made it so that once the JavaThread called `set_terminated(_thread_exiting)`
>> it no longer had to honor a suspend thread request.
>>
>> Summary: Yes, the `is_exiting()` call will result in detecting the exiting
>> JavaThread sooner than the `ThreadsListHandle::includes()` check.
>> However, it will not result in a change in suspend thread behavior.
>
> The `is_exiting` check seems unnecessary as the handshake code will not
> handshake with an exiting thread. The nested TLH was unnecessary too AFAICS.
U... The purpose of the new `is_exiting()` check and the baseline's
`ThreadsListHandle::includes()` check is to avoid making this call:
return this->handshake_state()->suspend();
The call we are avoiding is the one that makes the synchronous
SuspendThreadHandshake request (for threads other than self)
so by detecting the exiting thread early, we are avoiding entering
the handshake machinery.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Sun, 17 Oct 2021 12:45:59 GMT, David Holmes wrote:
>> I modeled the new check after the existing:
>>
>>
>> bool Thread::is_JavaThread_protected(const JavaThread* p) {
>>
>>
>> which is also a static function.
>
> While the name is somewhat ungainly - and unnecessarily detailed given
> `is_JavaThread_protected` has a similar constraint - it should be a static
> function as given because it must only be called on the current thread, and
> an instance method would give the false impression that it could be called on
> any thread.
>
> That said it should be possible to write that code block only once and reuse
> it. And the name as I said is somewhat ungainly. You could even have:
>
>
> static bool is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly =
> false) {
I'm checking out adding the `checkTLHOnly` param to `is_JavaThread_protected()`
and replacing `is_JavaThread_protected_by_my_ThreadsList()` calls with calls
to the updated function.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Sat, 16 Oct 2021 16:21:08 GMT, Daniel D. Daugherty
wrote:
>>> This seems an unrelated change in behaviour ??
>>
>> Actually this is equivalent code. The baseline code does this:
>>
>> ThreadsListHandle tlh;
>> if (!tlh.includes(this)) {
>> log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on
>> ThreadsList, no suspension", p2i(this));
>> return false;
>> }
>>
>>
>> What that code means is: if `this` thread does not appear in the newly
>> created
>> ThreadsListHandle's list, then `this` thread has called `java_suspend()`
>> after
>> `this` thread has exited. That's the only way that `this` thread could be
>> missing
>> from a newly created ThreadsListHandle's list.
>>
>> All I've done here is get rid of the ThreadsListHandle, verify that the
>> calling
>> context is protecting `this` and switch to an `is_exiting()` call.
>>
>>> So suspend_thread and resume thread's caller already takes a
>>> ThreadsListHandle
>>> so this is unnecessary and never happens.
>>
>> I presume @coleenp is saying that the `is_exiting()` check is not necessary.
>>
>> That might be true, it might not be true. I was trying to create equivalent
>> code without creating a nested ThreadsListHandle here and I think I've
>> done that. I actually think it might be possible for a JVM/TI event handler
>> to fire after the thread has set is_exiting() and for that event handler to
>> call SuspendThread() which could get us to this point.
>
> On rereading all of these comments and the current baseline code, I have
> to clarify one thing:
>
> There is a minor change in behavior caused by switching from a
> `ThreadsListHandle::includes()` check to a `JavaThread::is_exiting()`
> check. The `JavaThread::is_exiting()` will return true before the target
> JavaThread is removed from the system's current ThreadsList. So the
> `JavaThread::is_exiting()` check will return true faster and the
> `ThreadsListHandle::includes()` check.
>
> However, that change in behavior does not result in a change in
> behavior for a suspend thread request. Here's the relevant code:
>
>
> src/hotspot/share/runtime/thread.cpp:
> void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
>
>
> // The careful dance between thread suspension and exit is handled here.
> // Since we are in thread_in_vm state and suspension is done with
> handshakes,
> // we can just put in the exiting state and it will be correctly handled.
> set_terminated(_thread_exiting);
>
>
>
> // Remove from list of active threads list, and notify VM thread if we are
> the last non-daemon thread
> Threads::remove(this, daemon);
>
>
> }
>
>
> When we changed the suspend thread mechanism to use handshakes, we
> made it so that once the JavaThread called `set_terminated(_thread_exiting)`
> it no longer had to honor a suspend thread request.
>
> Summary: Yes, the `is_exiting()` call will result in detecting the exiting
> JavaThread sooner than the `ThreadsListHandle::includes()` check.
> However, it will not result in a change in suspend thread behavior.
The `is_exiting` check seems unnecessary as the handshake code will not
handshake with an exiting thread. The nested TLH was unnecessary too AFAICS.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 22:04:16 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/thread.cpp line 497:
>>
>>> 495: // placement somewhere in the calling context.
>>> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const
>>> JavaThread* p) {
>>> 497: Thread* current_thread = Thread::current();
>>
>> Shouldn't you call this on the current thread as "this" argument?
>
> I modeled the new check after the existing:
>
>
> bool Thread::is_JavaThread_protected(const JavaThread* p) {
>
>
> which is also a static function.
While the name is somewhat ungainly - and unnecessarily detailed given
`is_JavaThread_protected` has a similar constraint - it should be a static
function as given because it must only be called on the current thread, and an
instance method would give the false impression that it could be called on any
thread.
That said it should be possible to write that code block only once and reuse
it. And the name as I said is somewhat ungainly. You could even have:
static bool is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly =
false) {
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On 16/10/2021 8:26 am, Daniel D.Daugherty wrote:
On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes wrote:
Daniel D. Daugherty has updated the pull request incrementally with one
additional commit since the last revision:
8249004.cr1.patch
src/hotspot/share/prims/jvmtiEventController.cpp line 623:
621: // If we have a JvmtiThreadState, then we've reached the point where
622: // threads can exist so create a ThreadsListHandle to protect them.
623: ThreadsListHandle tlh;
Good catch on the missing TLH for this code.
It wasn't quite missing from the baseline code. This version of execute():
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
used to always create a ThreadsListHandle. I added a `ThreadsListHandle*`
parameter to that version and created a wrapper with the existing signature
to pass `nullptr` to the execute() version with the `ThreadsListHandle*`
parameter. What that means is that all existing callers of:
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
no longer had a ThreadsListHandle created for them. With the new sanity
check in place, I shook the trees to make sure that we had explicit
ThreadsListHandles in place for the locations that needed them.
`JvmtiEventControllerPrivate::recompute_enabled()` happened to be
one of the places where the ThreadsListHandle created by execute()
was hiding the fact that `recompute_enabled()` needed one.
Yup and that is exactly why I said good catch on finding the missing TLH.
Cheers,
David
src/hotspot/share/prims/jvmtiEventController.cpp line 624:
622: // threads can exist so create a ThreadsListHandle to protect them.
623: ThreadsListHandle tlh;
624: for (; state != NULL; state = state->next()) {
s/NULL/nullptr/
Missed that one. Fixed.
src/hotspot/share/runtime/handshake.cpp line 361:
359: } else {
360: if (tlh_p == nullptr) {
361: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
This should be an assert once this has had some bake time.
Agreed. All of the
`guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(),...`
calls should be changed to asserts down the road.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 22:19:15 GMT, Daniel D. Daugherty
wrote:
>> So suspend_thread and resume thread's caller already takes a
>> ThreadsListHandle so this is unnecessary and never happens.
>
>> This seems an unrelated change in behaviour ??
>
> Actually this is equivalent code. The baseline code does this:
>
> ThreadsListHandle tlh;
> if (!tlh.includes(this)) {
> log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on
> ThreadsList, no suspension", p2i(this));
> return false;
> }
>
>
> What that code means is: if `this` thread does not appear in the newly created
> ThreadsListHandle's list, then `this` thread has called `java_suspend()` after
> `this` thread has exited. That's the only way that `this` thread could be
> missing
> from a newly created ThreadsListHandle's list.
>
> All I've done here is get rid of the ThreadsListHandle, verify that the
> calling
> context is protecting `this` and switch to an `is_exiting()` call.
>
>> So suspend_thread and resume thread's caller already takes a
>> ThreadsListHandle
>> so this is unnecessary and never happens.
>
> I presume @coleenp is saying that the `is_exiting()` check is not necessary.
>
> That might be true, it might not be true. I was trying to create equivalent
> code without creating a nested ThreadsListHandle here and I think I've
> done that. I actually think it might be possible for a JVM/TI event handler
> to fire after the thread has set is_exiting() and for that event handler to
> call SuspendThread() which could get us to this point.
On rereading all of these comments and the current baseline code, I have
to clarify one thing:
There is a minor change in behavior caused by switching from a
`ThreadsListHandle::includes()` check to a `JavaThread::is_exiting()`
check. The `JavaThread::is_exiting()` will return true before the target
JavaThread is removed from the system's current ThreadsList. So the
`JavaThread::is_exiting()` check will return true faster and the
`ThreadsListHandle::includes()` check.
However, that change in behavior does not result in a change in
behavior for a suspend thread request. Here's the relevant code:
src/hotspot/share/runtime/thread.cpp:
void JavaThread::exit(bool destroy_vm, ExitType exit_type) {
// The careful dance between thread suspension and exit is handled here.
// Since we are in thread_in_vm state and suspension is done with
handshakes,
// we can just put in the exiting state and it will be correctly handled.
set_terminated(_thread_exiting);
// Remove from list of active threads list, and notify VM thread if we are
the last non-daemon thread
Threads::remove(this, daemon);
}
When we changed the suspend thread mechanism to use handshakes, we
made it so that once the JavaThread called `set_terminated(_thread_exiting)`
it no longer had to honor a suspend thread request.
Summary: Yes, the `is_exiting()` call will result in detecting the exiting
JavaThread sooner than the `ThreadsListHandle::includes()` check.
However, it will not result in a change in suspend thread behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 21:58:38 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/handshake.cpp line 358:
>>
>>> 356: bool target_is_dead = false;
>>> 357: if (target == nullptr) {
>>> 358: target_is_dead = true;
>>
>> Why would you pass a NULL target thread to Handshake::execute? Why would the
>> caller not check if the target is dead?
>
> The `NULL` target thread being passed in is actually handled by the baseline
> code:
>
>
> ThreadsListHandle tlh;
> if (tlh.includes(target)) {
>
>
> `tlh.includes(target)` returns `false` when `target` is `NULL/nullptr`.
> I just made the already handled situation more explicit.
>
>> Why would the caller not check if the target is dead?
>
> Hmmm... It's hard for me to answer that question since I didn't write
> the original code. The test code that calls `WB_HandshakeWalkStack()`
> or `WB_AsyncHandshakeWalkStack()` can call those functions with
> a `thread_handle` that translates into a `thread_oop` that returns a
> `NULL` `JavaThread*`.
>
> See the comment that I added to `WB_AsyncHandshakeWalkStack()` above.
Update: I've added comments to WB_HandshakeReadMonitors() and
WB_HandshakeWalkStack() to clarify their expectations.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v8]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: coleenp CR - add comments to clarify what WB_HandshakeReadMonitors() and WB_HandshakeWalkStack() expect. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/48416864..9518a9a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=06-07 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 06:54:10 GMT, David Holmes wrote: > This looks promising but I'm unclear on some of the details. I can't quite > work out > the criteria for deciding when to pass the TLH through to Handshake::execute. > If > it is passed through then the target is checked for being alive ie covered by > the > TLH. But if the TLH is null then it is assumed that the target is protected > elsewhere > up the stack and also assumed to be alive. I'm not sure why the two must go > together. For example given this code: > > ``` > ThreadsListHandle tlh(current_thread); > JavaThread *java_thread; > err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), > *thread_list, &java_thread, NULL); > GetSingleStackTraceClosure op(this, current_thread, *thread_list, > max_frame_count); > Handshake::execute(&op, &tlh, java_thread); > ``` > > what difference does it make if instead the last line is just: > > ` Handshake::execute(&op, java_thread);` > > ? How do I know which form should be used? Hmmm... This is a really good observation. When I was putting together this patch (the second time), my criteria was fairly simple: if we were calling `Handshake::execute()` and we happened to have a `ThreadsListHandle` in hand, then I switched to the new version of `Handshake::execute()` that takes a `ThreadsListHandle*`. As for what difference does it make, when we pass in a `ThreadsListHandle`, then we're saying "This is the `ThreadsListHandle` that protects the target JavaThread." and that's the _only_ `ThreadsListHandle` that we search. If we call the `Handshake::execute()` form that does not take a `ThreadsListHandle`, then we might end up searching all of the `ThreadsListHandles` that are associated with the calling thread. Since we stop on first match, we might only search the first one, but if the target JavaThread isn't in the first one, then we'll continue to search the next one and so on. Of course, down the road, if we end up changing code like this: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target), "missing ThreadsListHandle in calling context."); into an assert(), then we won't be doing any search in 'release' bits and then the `Handshake::execute()` that takes a `ThreadsListHandle*` will be more expensive in release bits. - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v7]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: dholmes CR - change NULL to nullptr. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/4e207e13..48416864 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 18:27:43 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/thread.cpp line 1771:
>>
>>> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
>>> 1770: "missing ThreadsListHandle in calling context.");
>>> 1771: if (is_exiting()) {
>>
>> This seems an unrelated change in behaviour ??
>
> So suspend_thread and resume thread's caller already takes a
> ThreadsListHandle so this is unnecessary and never happens.
> This seems an unrelated change in behaviour ??
Actually this is equivalent code. The baseline code does this:
ThreadsListHandle tlh;
if (!tlh.includes(this)) {
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on
ThreadsList, no suspension", p2i(this));
return false;
}
What that code means is: if `this` thread does not appear in the newly created
ThreadsListHandle's list, then `this` thread has called `java_suspend()` after
`this` thread has exited. That's the only way that `this` thread could be
missing
from a newly created ThreadsListHandle's list.
All I've done here is get rid of the ThreadsListHandle, verify that the calling
context is protecting `this` and switch to an `is_exiting()` call.
> So suspend_thread and resume thread's caller already takes a ThreadsListHandle
> so this is unnecessary and never happens.
I presume @coleenp is saying that the `is_exiting()` check is not necessary.
That might be true, it might not be true. I was trying to create equivalent
code without creating a nested ThreadsListHandle here and I think I've
done that. I actually think it might be possible for a JVM/TI event handler
to fire after the thread has set is_exiting() and for that event handler to
call SuspendThread() which could get us to this point.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 18:20:12 GMT, Coleen Phillimore wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/handshake.cpp line 358:
>
>> 356: bool target_is_dead = false;
>> 357: if (target == nullptr) {
>> 358: target_is_dead = true;
>
> Why would you pass a NULL target thread to Handshake::execute? Why would the
> caller not check if the target is dead?
The `NULL` target thread being passed in is actually handled by the baseline
code:
ThreadsListHandle tlh;
if (tlh.includes(target)) {
`tlh.includes(target)` returns `false` when `target` is `NULL/nullptr`.
I just made the already handled situation more explicit.
> Why would the caller not check if the target is dead?
Hmmm... It's hard for me to answer that question since I didn't write
the original code. The test code that calls `WB_HandshakeWalkStack()`
or `WB_AsyncHandshakeWalkStack()` can call those functions with
a `thread_handle` that translates into a `thread_oop` that returns a
`NULL` `JavaThread*`.
See the comment that I added to `WB_AsyncHandshakeWalkStack()` above.
> src/hotspot/share/runtime/thread.cpp line 497:
>
>> 495: // placement somewhere in the calling context.
>> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const
>> JavaThread* p) {
>> 497: Thread* current_thread = Thread::current();
>
> Shouldn't you call this on the current thread as "this" argument?
I modeled the new check after the existing:
bool Thread::is_JavaThread_protected(const JavaThread* p) {
which is also a static function.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/prims/jvmtiEventController.cpp line 623:
>
>> 621: // If we have a JvmtiThreadState, then we've reached the point where
>> 622: // threads can exist so create a ThreadsListHandle to protect them.
>> 623: ThreadsListHandle tlh;
>
> Good catch on the missing TLH for this code.
It wasn't quite missing from the baseline code. This version of execute():
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
used to always create a ThreadsListHandle. I added a `ThreadsListHandle*`
parameter to that version and created a wrapper with the existing signature
to pass `nullptr` to the execute() version with the `ThreadsListHandle*`
parameter. What that means is that all existing callers of:
`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`
no longer had a ThreadsListHandle created for them. With the new sanity
check in place, I shook the trees to make sure that we had explicit
ThreadsListHandles in place for the locations that needed them.
`JvmtiEventControllerPrivate::recompute_enabled()` happened to be
one of the places where the ThreadsListHandle created by execute()
was hiding the fact that `recompute_enabled()` needed one.
> src/hotspot/share/prims/jvmtiEventController.cpp line 624:
>
>> 622: // threads can exist so create a ThreadsListHandle to protect them.
>> 623: ThreadsListHandle tlh;
>> 624: for (; state != NULL; state = state->next()) {
>
> s/NULL/nullptr/
Missed that one. Fixed.
> src/hotspot/share/runtime/handshake.cpp line 361:
>
>> 359: } else {
>> 360: if (tlh_p == nullptr) {
>> 361:
>> guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
>
> This should be an assert once this has had some bake time.
Agreed. All of the
`guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(),...`
calls should be changed to asserts down the road.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Fri, 15 Oct 2021 06:45:02 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/thread.cpp line 1771:
>
>> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
>> 1770: "missing ThreadsListHandle in calling context.");
>> 1771: if (is_exiting()) {
>
> This seems an unrelated change in behaviour ??
So suspend_thread and resume thread's caller already takes a ThreadsListHandle
so this is unnecessary and never happens.
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty
wrote:
>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>>
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Daniel D. Daugherty has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8249004.cr1.patch
This has more moving pieces than the last version. I'm a bit uneasy about
passing NULL as a thread to Handshake::execute(). This shouldn't be something
that should happen.
src/hotspot/share/runtime/handshake.cpp line 358:
> 356: bool target_is_dead = false;
> 357: if (target == nullptr) {
> 358: target_is_dead = true;
Why would you pass a NULL target thread to Handshake::execute? Why would the
caller not check if the target is dead?
src/hotspot/share/runtime/thread.cpp line 497:
> 495: // placement somewhere in the calling context.
> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const JavaThread*
> p) {
> 497: Thread* current_thread = Thread::current();
Shouldn't you call this on the current thread as "this" argument?
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty
wrote:
>> A fix to reduce ThreadsListHandle overhead in relation to handshakes and
>> we add sanity checks for ThreadsListHandles higher in the call stack.
>>
>> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.
>
> Daniel D. Daugherty has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8249004.cr1.patch
Hi Dan,
This looks promising but I'm unclear on some of the details. I can't quite work
out the criteria for deciding when to pass the TLH through to
Handshake::execute. If it is passed through then the target is checked for
being alive ie covered by the TLH. But if the TLH is null then it is assumed
that the target is protected elsewhere up the stack and also assumed to be
alive. I'm not sure why the two must go together. For example given this code:
ThreadsListHandle tlh(current_thread);
JavaThread *java_thread;
err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(),
*thread_list, &java_thread, NULL);
GetSingleStackTraceClosure op(this, current_thread, *thread_list,
max_frame_count);
Handshake::execute(&op, &tlh, java_thread);
what difference does it make if instead the last line is just:
` Handshake::execute(&op, java_thread);`
? How do I know which form should be used?
Thanks,
David
src/hotspot/share/prims/jvmtiEventController.cpp line 623:
> 621: // If we have a JvmtiThreadState, then we've reached the point where
> 622: // threads can exist so create a ThreadsListHandle to protect them.
> 623: ThreadsListHandle tlh;
Good catch on the missing TLH for this code.
src/hotspot/share/prims/jvmtiEventController.cpp line 624:
> 622: // threads can exist so create a ThreadsListHandle to protect them.
> 623: ThreadsListHandle tlh;
> 624: for (; state != NULL; state = state->next()) {
s/NULL/nullptr/
src/hotspot/share/runtime/handshake.cpp line 361:
> 359: } else {
> 360: if (tlh_p == nullptr) {
> 361:
> guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
This should be an assert once this has had some bake time.
src/hotspot/share/runtime/thread.cpp line 1771:
> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
> 1770: "missing ThreadsListHandle in calling context.");
> 1771: if (is_exiting()) {
This seems an unrelated change in behaviour ??
-
PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote: >> A fix to reduce ThreadsListHandle overhead in relation to handshakes and >> we add sanity checks for ThreadsListHandles higher in the call stack. >> >> This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. > > Hi Dan, > > I just updated the bug report. This really isn't addressing the reasons the > RFE was filed. > > David @dholmes-ora, @coleenp and @sspitsyn - I've finished reworking the patch and I'm finally happy with how it works. This version has passed Mach5 Tier[1-7] and has mostly finished Mach5 Tier8 (it's in the 24-hour test phase)... - PR: https://git.openjdk.java.net/jdk/pull/4677
Re: RFR: 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes [v6]
> A fix to reduce ThreadsListHandle overhead in relation to handshakes and > we add sanity checks for ThreadsListHandles higher in the call stack. > > This fix was tested with Mach5 Tier[1-8]; Tier8 is still running. Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision: 8249004.cr1.patch - Changes: - all: https://git.openjdk.java.net/jdk/pull/4677/files - new: https://git.openjdk.java.net/jdk/pull/4677/files/a214b284..4e207e13 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4677&range=04-05 Stats: 83 lines in 5 files changed: 65 ins; 2 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/4677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4677/head:pull/4677 PR: https://git.openjdk.java.net/jdk/pull/4677
