8021335: Missing synchronization when reading counters for live threads and peak thread count
Reviewed-by: dholmes, mchung
--- a/src/hotspot/share/runtime/thread.cpp Thu Oct 25 17:06:40 2018 -0700
+++ b/src/hotspot/share/runtime/thread.cpp Thu Oct 25 18:41:26 2018 -0700
@@ -1819,6 +1819,9 @@
thread->clear_pending_exception();
}
+static bool is_daemon(oop threadObj) {
+ return (threadObj != NULL && java_lang_Thread::is_daemon(threadObj));
+}
// For any new cleanup additions, please check to see if they need to be applied to
// cleanup_failed_attach_current_thread as well.
@@ -1910,7 +1913,7 @@
MutexLockerEx ml(SR_lock(), Mutex::_no_safepoint_check_flag);
if (!is_external_suspend()) {
set_terminated(_thread_exiting);
- ThreadService::current_thread_exiting(this);
+ ThreadService::current_thread_exiting(this, is_daemon(threadObj()));
break;
}
// Implied else:
@@ -1930,6 +1933,7 @@
}
// no more external suspends are allowed at this point
} else {
+ assert(!is_terminated() && !is_exiting(), "must not be exiting");
// before_exit() has already posted JVMTI THREAD_END events
}
@@ -4332,7 +4336,7 @@
void Threads::add(JavaThread* p, bool force_daemon) {
// The threads lock must be owned at this point
- assert_locked_or_safepoint(Threads_lock);
+ assert(Threads_lock->owned_by_self(), "must have threads lock");
BarrierSet::barrier_set()->on_thread_attach(p);
@@ -4348,7 +4352,7 @@
bool daemon = true;
// Bootstrapping problem: threadObj can be null for initial
// JavaThread (or for threads attached via JNI)
- if ((!force_daemon) && (threadObj == NULL || !java_lang_Thread::is_daemon(threadObj))) {
+ if ((!force_daemon) && !is_daemon((threadObj))) {
_number_of_non_daemon_threads++;
daemon = false;
}
@@ -4393,7 +4397,7 @@
_number_of_threads--;
oop threadObj = p->threadObj();
bool daemon = true;
- if (threadObj == NULL || !java_lang_Thread::is_daemon(threadObj)) {
+ if (!is_daemon(threadObj)) {
_number_of_non_daemon_threads--;
daemon = false;
--- a/src/hotspot/share/services/threadService.cpp Thu Oct 25 17:06:40 2018 -0700
+++ b/src/hotspot/share/services/threadService.cpp Thu Oct 25 18:41:26 2018 -0700
@@ -57,8 +57,8 @@
PerfVariable* ThreadService::_live_threads_count = NULL;
PerfVariable* ThreadService::_peak_threads_count = NULL;
PerfVariable* ThreadService::_daemon_threads_count = NULL;
-volatile int ThreadService::_exiting_threads_count = 0;
-volatile int ThreadService::_exiting_daemon_threads_count = 0;
+volatile int ThreadService::_atomic_threads_count = 0;
+volatile int ThreadService::_atomic_daemon_threads_count = 0;
ThreadDumpResult* ThreadService::_threaddump_list = NULL;
@@ -101,50 +101,104 @@
_peak_threads_count->set_value(get_live_thread_count());
}
+static bool is_hidden_thread(JavaThread *thread) {
+ // hide VM internal or JVMTI agent threads
+ return thread->is_hidden_from_external_view() || thread->is_jvmti_agent_thread();
+}
+
void ThreadService::add_thread(JavaThread* thread, bool daemon) {
- // Do not count VM internal or JVMTI agent threads
- if (thread->is_hidden_from_external_view() ||
- thread->is_jvmti_agent_thread()) {
+ assert(Threads_lock->owned_by_self(), "must have threads lock");
+
+ // Do not count hidden threads
+ if (is_hidden_thread(thread)) {
return;
}
_total_threads_count->inc();
_live_threads_count->inc();
+ Atomic::inc(&_atomic_threads_count);
+ int count = _atomic_threads_count;
- if (_live_threads_count->get_value() > _peak_threads_count->get_value()) {
- _peak_threads_count->set_value(_live_threads_count->get_value());
+ if (count > _peak_threads_count->get_value()) {
+ _peak_threads_count->set_value(count);
}
if (daemon) {
_daemon_threads_count->inc();
+ Atomic::inc(&_atomic_daemon_threads_count);
+ }
+}
+
+void ThreadService::decrement_thread_counts(JavaThread* jt, bool daemon) {
+ Atomic::dec(&_atomic_threads_count);
+
+ if (daemon) {
+ Atomic::dec(&_atomic_daemon_threads_count);
}
}
void ThreadService::remove_thread(JavaThread* thread, bool daemon) {
- Atomic::dec(&_exiting_threads_count);
- if (daemon) {
- Atomic::dec(&_exiting_daemon_threads_count);
- }
+ assert(Threads_lock->owned_by_self(), "must have threads lock");
- if (thread->is_hidden_from_external_view() ||
- thread->is_jvmti_agent_thread()) {
+ // Do not count hidden threads
+ if (is_hidden_thread(thread)) {
return;
}
- _live_threads_count->set_value(_live_threads_count->get_value() - 1);
+ assert(!thread->is_terminated(), "must not be terminated");
+ if (!thread->is_exiting()) {
+ // JavaThread::exit() skipped calling current_thread_exiting()
+ decrement_thread_counts(thread, daemon);
+ }
+
+ int daemon_count = _atomic_daemon_threads_count;
+ int count = _atomic_threads_count;
+
+ // Counts are incremented at the same time, but atomic counts are
+ // decremented earlier than perf counts.
+ assert(_live_threads_count->get_value() > count,
+ "thread count mismatch %d : %d",
+ (int)_live_threads_count->get_value(), count);
+
+ _live_threads_count->dec(1);
if (daemon) {
- _daemon_threads_count->set_value(_daemon_threads_count->get_value() - 1);
+ assert(_daemon_threads_count->get_value() > daemon_count,
+ "thread count mismatch %d : %d",
+ (int)_daemon_threads_count->get_value(), daemon_count);
+
+ _daemon_threads_count->dec(1);
}
+
+ // Counts are incremented at the same time, but atomic counts are
+ // decremented earlier than perf counts.
+ assert(_daemon_threads_count->get_value() >= daemon_count,
+ "thread count mismatch %d : %d",
+ (int)_daemon_threads_count->get_value(), daemon_count);
+ assert(_live_threads_count->get_value() >= count,
+ "thread count mismatch %d : %d",
+ (int)_live_threads_count->get_value(), count);
+ assert(_live_threads_count->get_value() > 0 ||
+ (_live_threads_count->get_value() == 0 && count == 0 &&
+ _daemon_threads_count->get_value() == 0 && daemon_count == 0),
+ "thread counts should reach 0 at the same time, live %d,%d daemon %d,%d",
+ (int)_live_threads_count->get_value(), count,
+ (int)_daemon_threads_count->get_value(), daemon_count);
+ assert(_daemon_threads_count->get_value() > 0 ||
+ (_daemon_threads_count->get_value() == 0 && daemon_count == 0),
+ "thread counts should reach 0 at the same time, daemon %d,%d",
+ (int)_daemon_threads_count->get_value(), daemon_count);
}
-void ThreadService::current_thread_exiting(JavaThread* jt) {
- assert(jt == JavaThread::current(), "Called by current thread");
- Atomic::inc(&_exiting_threads_count);
+void ThreadService::current_thread_exiting(JavaThread* jt, bool daemon) {
+ // Do not count hidden threads
+ if (is_hidden_thread(jt)) {
+ return;
+ }
- oop threadObj = jt->threadObj();
- if (threadObj != NULL && java_lang_Thread::is_daemon(threadObj)) {
- Atomic::inc(&_exiting_daemon_threads_count);
- }
+ assert(jt == JavaThread::current(), "Called by current thread");
+ assert(!jt->is_terminated() && jt->is_exiting(), "must be exiting");
+
+ decrement_thread_counts(jt, daemon);
}
// FIXME: JVMTI should call this function
--- a/src/hotspot/share/services/threadService.hpp Thu Oct 25 17:06:40 2018 -0700
+++ b/src/hotspot/share/services/threadService.hpp Thu Oct 25 18:41:26 2018 -0700
@@ -58,10 +58,12 @@
static PerfVariable* _peak_threads_count;
static PerfVariable* _daemon_threads_count;
- // These 2 counters are atomically incremented once the thread is exiting.
- // They will be atomically decremented when ThreadService::remove_thread is called.
- static volatile int _exiting_threads_count;
- static volatile int _exiting_daemon_threads_count;
+ // These 2 counters are like the above thread counts, but are
+ // atomically decremented in ThreadService::current_thread_exiting instead of
+ // ThreadService::remove_thread, so that the thread count is updated before
+ // Thread.join() returns.
+ static volatile int _atomic_threads_count;
+ static volatile int _atomic_daemon_threads_count;
static bool _thread_monitoring_contention_enabled;
static bool _thread_cpu_time_enabled;
@@ -72,11 +74,13 @@
// requested by multiple threads concurrently.
static ThreadDumpResult* _threaddump_list;
+ static void decrement_thread_counts(JavaThread* jt, bool daemon);
+
public:
static void init();
static void add_thread(JavaThread* thread, bool daemon);
static void remove_thread(JavaThread* thread, bool daemon);
- static void current_thread_exiting(JavaThread* jt);
+ static void current_thread_exiting(JavaThread* jt, bool daemon);
static bool set_thread_monitoring_contention(bool flag);
static bool is_thread_monitoring_contention() { return _thread_monitoring_contention_enabled; }
@@ -89,11 +93,8 @@
static jlong get_total_thread_count() { return _total_threads_count->get_value(); }
static jlong get_peak_thread_count() { return _peak_threads_count->get_value(); }
- static jlong get_live_thread_count() { return _live_threads_count->get_value() - _exiting_threads_count; }
- static jlong get_daemon_thread_count() { return _daemon_threads_count->get_value() - _exiting_daemon_threads_count; }
-
- static int exiting_threads_count() { return _exiting_threads_count; }
- static int exiting_daemon_threads_count() { return _exiting_daemon_threads_count; }
+ static jlong get_live_thread_count() { return _atomic_threads_count; }
+ static jlong get_daemon_thread_count() { return _atomic_daemon_threads_count; }
// Support for thread dump
static void add_thread_dump(ThreadDumpResult* dump);
--- a/test/jdk/java/lang/management/ThreadMXBean/ResetPeakThreadCount.java Thu Oct 25 17:06:40 2018 -0700
+++ b/test/jdk/java/lang/management/ThreadMXBean/ResetPeakThreadCount.java Thu Oct 25 18:41:26 2018 -0700
@@ -158,10 +158,6 @@
" Expected to be == previous peak = " + peak1 + " + " +
delta);
}
- // wait until the current thread count gets incremented
- while (mbean.getThreadCount() < (current + count)) {
- Thread.sleep(100);
- }
current = mbean.getThreadCount();
System.out.println(" Live thread count before returns " + current);
return current;
@@ -195,12 +191,6 @@
allThreads[i].join();
}
- // there is a race in the counter update logic somewhere causing
- // the thread counters go ff
- // we need to give the terminated threads some extra time to really die
- // JDK-8021335
- Thread.sleep(500);
-
long current = mbean.getThreadCount();
System.out.println(" Live thread count before returns " + current);
return current;