New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes #4677
Changes from 1 commit
8162b22
bc82cfa
11f5976
507bcbc
093ad30
a214b28
4e207e1
4841686
9518a9a
045b3e0
3e1d1b0
86fcdfb
9117350
fe28cbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
@@ -616,8 +616,14 @@ JvmtiEventControllerPrivate::recompute_enabled() { | ||
} | ||
|
||
// compute and set thread-filtered events | ||
for (JvmtiThreadState *state = JvmtiThreadState::first(); state != NULL; state = state->next()) { | ||
any_env_thread_enabled |= recompute_thread_enabled(state); | ||
JvmtiThreadState *state = JvmtiThreadState::first(); | ||
if (state != nullptr) { | ||
// If we have a JvmtiThreadState, then we've reached the point where | ||
// threads can exist so create a ThreadsListHandle to protect them. | ||
ThreadsListHandle tlh; | ||
for (; state != NULL; state = state->next()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/NULL/nullptr/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed that one. Fixed. |
||
any_env_thread_enabled |= recompute_thread_enabled(state); | ||
} | ||
} | ||
|
||
// set universal state (across all envs and threads) | ||
@@ -342,8 +342,9 @@ void Handshake::execute(HandshakeClosure* hs_cl) { | ||
} | ||
|
||
void Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target) { | ||
ThreadsListHandle tlh; | ||
Handshake::execute(hs_cl, &tlh, target); | ||
// tlh_p == nullptr means we rely on a ThreadsListHandle somewhere | ||
// in the caller's context (and we sanity check for that). | ||
Handshake::execute(hs_cl, nullptr, target); | ||
} | ||
|
||
void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh_p, JavaThread* target) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can we drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in handshake.[ch]pp. |
||
@@ -352,9 +353,21 @@ void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh_p, JavaT | ||
|
||
jlong start_time_ns = os::javaTimeNanos(); | ||
|
||
if (tlh_p->includes(target)) { | ||
target->handshake_state()->add_operation(&op); | ||
bool target_is_dead = false; | ||
if (target == nullptr) { | ||
target_is_dead = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you pass a NULL target thread to Handshake::execute? Why would the caller not check if the target is dead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Hmmm... It's hard for me to answer that question since I didn't write See the comment that I added to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: I've added comments to WB_HandshakeReadMonitors() and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update again: I took a closer look at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great. |
||
} else { | ||
if (tlh_p == nullptr) { | ||
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be an assert once this has had some bake time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. All of the |
||
"missing ThreadsListHandle in calling context."); | ||
target->handshake_state()->add_operation(&op); | ||
} else if (tlh_p->includes(target)) { | ||
target->handshake_state()->add_operation(&op); | ||
} else { | ||
target_is_dead = true; | ||
} | ||
} | ||
if (target_is_dead) { | ||
char buf[128]; | ||
jio_snprintf(buf, sizeof(buf), "(thread= " INTPTR_FORMAT " dead)", p2i(target)); | ||
log_handshake_info(start_time_ns, op.name(), 0, 0, buf); | ||
@@ -400,13 +413,24 @@ void Handshake::execute(AsyncHandshakeClosure* hs_cl, JavaThread* target) { | ||
jlong start_time_ns = os::javaTimeNanos(); | ||
AsyncHandshakeOperation* op = new AsyncHandshakeOperation(hs_cl, target, start_time_ns); | ||
|
||
ThreadsListHandle tlh; | ||
if (tlh.includes(target)) { | ||
target->handshake_state()->add_operation(op); | ||
} else { | ||
if (target == nullptr) { | ||
// Stress test that calls WB_AsyncHandshakeWalkStack() can get here: | ||
log_handshake_info(start_time_ns, op->name(), 0, 0, "(thread dead)"); | ||
delete op; | ||
return; | ||
} | ||
|
||
Thread* current = Thread::current(); | ||
if (current != target) { | ||
// Another thread is handling the request and it must be protecting | ||
// the target. | ||
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target), | ||
"missing ThreadsListHandle in calling context."); | ||
} | ||
// Implied else: | ||
// The target is handling the request itself so it can't be dead. | ||
|
||
target->handshake_state()->add_operation(op); | ||
} | ||
|
||
HandshakeState::HandshakeState(JavaThread* target) : | ||
@@ -486,6 +486,29 @@ bool Thread::is_JavaThread_protected(const JavaThread* p) { | ||
return false; | ||
} | ||
|
||
// Is the target JavaThread protected by a ThreadsList associated | ||
// with the calling Thread. | ||
// | ||
// Thread::is_JavaThread_protected() above is the more general check. | ||
// This function ONLY checks the ThreadsLists (if any) associated with | ||
// the calling thread in order to verify proper ThreadsListHandle | ||
// placement somewhere in the calling context. | ||
bool Thread::is_JavaThread_protected_by_my_ThreadsList(const JavaThread* p) { | ||
Thread* current_thread = Thread::current(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't you call this on the current thread as "this" argument? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I modeled the new check after the existing:
which is also a static function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the name is somewhat ungainly - and unnecessarily detailed given 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm checking out adding the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing went well so the above change will be in the next change that I push. |
||
// Check the ThreadsLists associated with the calling thread (if any) | ||
// to see if one of them protects the target JavaThread: | ||
for (SafeThreadsListPtr* stlp = current_thread->_threads_list_ptr; | ||
stlp != NULL; stlp = stlp->previous()) { | ||
if (stlp->list()->includes(p)) { | ||
// The target JavaThread is protected by this ThreadsList: | ||
return true; | ||
} | ||
} | ||
|
||
// The target JavaThread is not protected. | ||
return false; | ||
} | ||
|
||
ThreadPriority Thread::get_priority(const Thread* const thread) { | ||
ThreadPriority priority; | ||
// Can return an error! | ||
@@ -1743,18 +1766,20 @@ void JavaThread::send_thread_stop(oop java_throwable) { | ||
// - Target thread will not enter any new monitors. | ||
// | ||
bool JavaThread::java_suspend() { | ||
ThreadsListHandle tlh; | ||
if (!tlh.includes(this)) { | ||
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, no suspension", p2i(this)); | ||
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this), | ||
"missing ThreadsListHandle in calling context."); | ||
if (is_exiting()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems an unrelated change in behaviour ?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So suspend_thread and resume thread's caller already takes a ThreadsListHandle so this is unnecessary and never happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually this is equivalent code. The baseline code does this:
What that code means is: if All I've done here is get rid of the ThreadsListHandle, verify that the calling
I presume @coleenp is saying that the That might be true, it might not be true. I was trying to create equivalent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On rereading all of these comments and the current baseline code, I have There is a minor change in behavior caused by switching from a However, that change in behavior does not result in a change in
When we changed the suspend thread mechanism to use handshakes, we Summary: Yes, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ummmm... The purpose of the new
The call we are avoiding is the one that makes the synchronous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay you've convinced me. I'm merging my latest patch with master |
||
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " is exiting, no suspension", p2i(this)); | ||
return false; | ||
} | ||
return this->handshake_state()->suspend(); | ||
} | ||
|
||
bool JavaThread::java_resume() { | ||
ThreadsListHandle tlh; | ||
if (!tlh.includes(this)) { | ||
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, nothing to resume", p2i(this)); | ||
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this), | ||
"missing ThreadsListHandle in calling context."); | ||
if (is_exiting()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we remove this the same as we did for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rationale for removing the is_exiting() check from If we look at Here's the code:
I'm not seeing anything in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A suspended thread cannot be exiting - else the suspend logic is broken. So, given you can call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! I have to keep reminding myself that with handshake based suspend |
||
log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " is exiting, nothing to resume", p2i(this)); | ||
return false; | ||
} | ||
return this->handshake_state()->resume(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the missing TLH for this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theThreadsListHandle*
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 beone of the places where the ThreadsListHandle created by execute()
was hiding the fact that
recompute_enabled()
needed one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ThreadsListHandle
protectsJavaThread
objects notJvmtiThreadState
objects.JvmtiThreadState::first()
returns the head of the global list ofJvmtiThreadState
objects for the system. Each
JvmtiThreadState
object contains aJavaThread*
andwe have to protect use of the
JavaThread*
which can happen in therecompute_thread_enabled(state)
call below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JvmtiThreadState objects point to JavaThread and vice versa, so I still don't see why you don't protect the first element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've written up a rather long analysis about how the use of
JvmtiThreadState_lock
in
JvmtiEventControllerPrivate::recompute_enabled()
means that we can safelytraverse the JvmtiThreadState list returned by
JvmtiThreadState::first()
withoutracing with an exiting JavaThread. I've sent it to you via direct email.
I could amend the comment above the ThreadsListHandle like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment has been pushed to this PR.