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 12 commits
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -435,30 +435,38 @@ void Thread::check_for_dangling_thread_pointer(Thread *thread) { | ||
} | ||
#endif | ||
|
||
// Is the target JavaThread protected by the calling Thread | ||
// or by some other mechanism: | ||
bool Thread::is_JavaThread_protected(const JavaThread* p) { | ||
// Do the simplest check first: | ||
if (SafepointSynchronize::is_at_safepoint()) { | ||
// The target is protected since JavaThreads cannot exit | ||
// while we're at a safepoint. | ||
return true; | ||
} | ||
// Is the target JavaThread protected by the calling Thread or by some other | ||
// mechanism? 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. | ||
// | ||
bool Thread::is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly) { | ||
Thread* current_thread = nullptr; | ||
if (checkTLHOnly) { | ||
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. This seems redundant due to line 463. You can just have a 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 suspect that the way that git is displaying the diffs is confusing you. We need I could simplify the logic by always setting current thread when it is 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. 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. 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 really tempted to go ahead and change it to always set 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, ^ 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? 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've changed 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.
See the function's header comment:
I had a copy of this logic in a separate function and @dholmes-ora suggested I could move the logic into a separate function and then call that new function
Please read the header comment to see what |
||
} else { | ||
// Do the simplest check first: | ||
if (SafepointSynchronize::is_at_safepoint()) { | ||
// The target is protected since JavaThreads cannot exit | ||
// while we're at a safepoint. | ||
return true; | ||
} | ||
|
||
// If the target hasn't been started yet then it is trivially | ||
// "protected". We assume the caller is the thread that will do | ||
// the starting. | ||
if (p->osthread() == NULL || p->osthread()->get_state() <= INITIALIZED) { | ||
return true; | ||
} | ||
// If the target hasn't been started yet then it is trivially | ||
// "protected". We assume the caller is the thread that will do | ||
// the starting. | ||
if (p->osthread() == NULL || p->osthread()->get_state() <= INITIALIZED) { | ||
return true; | ||
} | ||
|
||
// Now make the simple checks based on who the caller is: | ||
Thread* current_thread = Thread::current(); | ||
if (current_thread == p || Threads_lock->owner() == current_thread) { | ||
// Target JavaThread is self or calling thread owns the Threads_lock. | ||
// Second check is the same as Threads_lock->owner_is_self(), | ||
// but we already have the current thread so check directly. | ||
return true; | ||
// Now make the simple checks based on who the caller is: | ||
current_thread = Thread::current(); | ||
if (current_thread == p || Threads_lock->owner() == current_thread) { | ||
// Target JavaThread is self or calling thread owns the Threads_lock. | ||
// Second check is the same as Threads_lock->owner_is_self(), | ||
// but we already have the current thread so check directly. | ||
return true; | ||
} | ||
} | ||
|
||
// Check the ThreadsLists associated with the calling thread (if any) | ||
@@ -471,16 +479,18 @@ bool Thread::is_JavaThread_protected(const JavaThread* p) { | ||
} | ||
} | ||
|
||
// Use this debug code with -XX:+UseNewCode to diagnose locations that | ||
// are missing a ThreadsListHandle or other protection mechanism: | ||
// guarantee(!UseNewCode, "current_thread=" INTPTR_FORMAT " is not protecting p=" | ||
// INTPTR_FORMAT, p2i(current_thread), p2i(p)); | ||
if (!checkTLHOnly) { | ||
// Use this debug code with -XX:+UseNewCode to diagnose locations that | ||
// are missing a ThreadsListHandle or other protection mechanism: | ||
// guarantee(!UseNewCode, "current_thread=" INTPTR_FORMAT " is not protecting p=" | ||
// INTPTR_FORMAT, p2i(current_thread), p2i(p)); | ||
|
||
// Note: Since 'p' isn't protected by a TLH, the call to | ||
// p->is_handshake_safe_for() may crash, but we have debug bits so | ||
// we'll be able to figure out what protection mechanism is missing. | ||
assert(p->is_handshake_safe_for(current_thread), "JavaThread=" INTPTR_FORMAT | ||
" is not protected and not handshake safe.", p2i(p)); | ||
// Note: Since 'p' isn't protected by a TLH, the call to | ||
// p->is_handshake_safe_for() may crash, but we have debug bits so | ||
// we'll be able to figure out what protection mechanism is missing. | ||
assert(p->is_handshake_safe_for(current_thread), "JavaThread=" INTPTR_FORMAT | ||
" is not protected and not handshake safe.", p2i(p)); | ||
} | ||
|
||
// The target JavaThread is not protected so it is not safe to query: | ||
return false; | ||
@@ -1743,20 +1753,14 @@ 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)); | ||
return false; | ||
} | ||
guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), | ||
"missing ThreadsListHandle in calling context."); | ||
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)); | ||
return false; | ||
} | ||
guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true), | ||
"missing ThreadsListHandle in calling context."); | ||
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.