Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes #4677

Closed
wants to merge 14 commits into from
Closed
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1560,7 +1560,7 @@ JvmtiEnv::GetThreadListStackTraces(jint thread_count, const jthread* thread_list
}

GetSingleStackTraceClosure op(this, current_thread, *thread_list, max_frame_count);
Handshake::execute(&op, java_thread);
Handshake::execute(&op, &tlh, java_thread);
err = op.result();
if (err == JVMTI_ERROR_NONE) {
*stack_info_ptr = op.stack_info();
@@ -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;
Copy link
Member

@dholmes-ora dholmes-ora Oct 15, 2021

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.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2021

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

Copy link
Contributor

@coleenp coleenp Nov 4, 2021

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.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@coleenp coleenp Nov 5, 2021

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.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 5, 2021

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 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;

Copy link
Contributor

@coleenp coleenp Nov 5, 2021

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.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 5, 2021

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.

for (; state != nullptr; state = state->next()) {
any_env_thread_enabled |= recompute_thread_enabled(state);
}
}

// set universal state (across all envs and threads)
@@ -2070,10 +2070,13 @@ WB_ENTRY(jboolean, WB_HandshakeReadMonitors(JNIEnv* env, jobject wb, jobject thr
};

ReadMonitorsClosure rmc;
oop thread_oop = JNIHandles::resolve(thread_handle);
if (thread_oop != NULL) {
JavaThread* target = java_lang_Thread::thread(thread_oop);
Handshake::execute(&rmc, target);
if (thread_handle != NULL) {
ThreadsListHandle tlh;
JavaThread* target = nullptr;
bool is_alive = tlh.cv_internal_thread_to_JavaThread(thread_handle, &target, NULL);
if (is_alive) {
Handshake::execute(&rmc, &tlh, target);
}
}
return rmc.executed();
WB_END
@@ -2101,11 +2104,12 @@ WB_ENTRY(jint, WB_HandshakeWalkStack(JNIEnv* env, jobject wb, jobject thread_han

if (all_threads) {
Handshake::execute(&tsc);
} else {
oop thread_oop = JNIHandles::resolve(thread_handle);
if (thread_oop != NULL) {
JavaThread* target = java_lang_Thread::thread(thread_oop);
Handshake::execute(&tsc, target);
} else if (thread_handle != NULL) {
ThreadsListHandle tlh;
JavaThread* target = nullptr;
bool is_alive = tlh.cv_internal_thread_to_JavaThread(thread_handle, &target, NULL);
if (is_alive) {
Handshake::execute(&tsc, &tlh, target);
}
}
return tsc.num_threads_completed();
@@ -2129,11 +2133,14 @@ WB_ENTRY(void, WB_AsyncHandshakeWalkStack(JNIEnv* env, jobject wb, jobject threa
public:
TraceSelfClosure(JavaThread* self_target) : AsyncHandshakeClosure("WB_TraceSelf"), _self(self_target) {}
};
oop thread_oop = JNIHandles::resolve(thread_handle);
if (thread_oop != NULL) {
JavaThread* target = java_lang_Thread::thread(thread_oop);
TraceSelfClosure* tsc = new TraceSelfClosure(target);
Handshake::execute(tsc, target);
if (thread_handle != NULL) {
ThreadsListHandle tlh;
JavaThread* target = nullptr;
bool is_alive = tlh.cv_internal_thread_to_JavaThread(thread_handle, &target, NULL);
if (is_alive) {
TraceSelfClosure* tsc = new TraceSelfClosure(target);
Handshake::execute(tsc, target);
}
}
WB_END

@@ -342,13 +342,23 @@ void Handshake::execute(HandshakeClosure* hs_cl) {
}

void Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target) {
// tlh == 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, JavaThread* target) {
JavaThread* self = JavaThread::current();
HandshakeOperation op(hs_cl, target, Thread::current());

jlong start_time_ns = os::javaTimeNanos();

ThreadsListHandle tlh;
if (tlh.includes(target)) {
guarantee(target != nullptr, "must be");
if (tlh == nullptr) {
guarantee(Thread::is_JavaThread_protected(target, /* checkTLHOnly */ true),
"missing ThreadsListHandle in calling context.");
target->handshake_state()->add_operation(&op);
} else if (tlh->includes(target)) {
target->handshake_state()->add_operation(&op);
} else {
char buf[128];
@@ -396,13 +406,19 @@ 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 {
log_handshake_info(start_time_ns, op->name(), 0, 0, "(thread dead)");
delete op;
guarantee(target != nullptr, "must be");

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(target, /* checkTLHOnly */ true),
"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) :
@@ -36,6 +36,7 @@ class HandshakeOperation;
class JavaThread;
class SuspendThreadHandshake;
class ThreadSelfSuspensionHandshake;
class ThreadsListHandle;

// A handshake closure is a callback that is executed for a JavaThread
// while it is in a safepoint/handshake-safe state. Depending on the
@@ -64,7 +65,16 @@ class Handshake : public AllStatic {
public:
// Execution of handshake operation
static void execute(HandshakeClosure* hs_cl);
// This version of execute() relies on a ThreadListHandle somewhere in
// the caller's context to protect target (and we sanity check for that).
static void execute(HandshakeClosure* hs_cl, JavaThread* target);
// This version of execute() is used when you have a ThreadListHandle in
// hand and are using it to protect target. If tlh == nullptr, then we
// sanity check for a ThreadListHandle somewhere in the caller's context
// to verify that target is protected.
static void execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh, JavaThread* target);
// This version of execute() relies on a ThreadListHandle somewhere in
// the caller's context to protect target (and we sanity check for that).
static void execute(AsyncHandshakeClosure* hs_cl, JavaThread* target);
};

@@ -435,30 +435,35 @@ 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 = Thread::current();
if (!checkTLHOnly) {
// 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:
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 +476,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 +1750,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();
}

@@ -199,9 +199,11 @@ class Thread: public ThreadShadow {
}

public:
// Is the target JavaThread protected by the calling Thread
// or by some other mechanism:
static bool is_JavaThread_protected(const JavaThread* p);
// 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.
static bool is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly = false);

void* operator new(size_t size) throw() { return allocate(size, true); }
void* operator new(size_t size, const std::nothrow_t& nothrow_constant) throw() {