8209976: Improve iteration over non-JavaThreads
authorkbarrett
Tue, 28 Aug 2018 16:04:54 -0400
changeset 51548 35a6956f4243
parent 51547 2b004d807187
child 51549 f419dbb34719
8209976: Improve iteration over non-JavaThreads Summary: Add NonJavaThread and move NamedThread iteration to new class. Reviewed-by: eosterlund, coleenp, rkennke
src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
src/hotspot/share/runtime/mutexLocker.cpp
src/hotspot/share/runtime/mutexLocker.hpp
src/hotspot/share/runtime/thread.cpp
src/hotspot/share/runtime/thread.hpp
src/hotspot/share/runtime/vmStructs.cpp
src/hotspot/share/utilities/globalCounter.cpp
src/hotspot/share/utilities/globalCounter.inline.hpp
--- a/src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp	Tue Aug 28 16:04:54 2018 -0400
@@ -308,7 +308,7 @@
   _added_native(0) {
 }
 
-class JfrThreadSampler : public Thread {
+class JfrThreadSampler : public NonJavaThread {
   friend class JfrThreadSampling;
  private:
   Semaphore _sample;
--- a/src/hotspot/share/runtime/mutexLocker.cpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/runtime/mutexLocker.cpp	Tue Aug 28 16:04:54 2018 -0400
@@ -76,7 +76,7 @@
 Monitor* Safepoint_lock               = NULL;
 Monitor* SerializePage_lock           = NULL;
 Monitor* Threads_lock                 = NULL;
-Mutex*   NamedThreadsList_lock        = NULL;
+Mutex*   NonJavaThreadsList_lock      = NULL;
 Monitor* CGC_lock                     = NULL;
 Monitor* STS_lock                     = NULL;
 Monitor* FullGCCount_lock             = NULL;
@@ -257,7 +257,7 @@
   def(Safepoint_lock               , PaddedMonitor, safepoint,   true,  Monitor::_safepoint_check_sometimes);  // locks SnippetCache_lock/Threads_lock
 
   def(Threads_lock                 , PaddedMonitor, barrier,     true,  Monitor::_safepoint_check_sometimes);
-  def(NamedThreadsList_lock        , PaddedMutex,   leaf,        true,  Monitor::_safepoint_check_never);
+  def(NonJavaThreadsList_lock      , PaddedMutex,   leaf,        true,  Monitor::_safepoint_check_never);
 
   def(VMOperationQueue_lock        , PaddedMonitor, nonleaf,     true,  Monitor::_safepoint_check_sometimes);  // VM_thread allowed to block on these
   def(VMOperationRequest_lock      , PaddedMonitor, nonleaf,     true,  Monitor::_safepoint_check_sometimes);
--- a/src/hotspot/share/runtime/mutexLocker.hpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/runtime/mutexLocker.hpp	Tue Aug 28 16:04:54 2018 -0400
@@ -72,7 +72,7 @@
 extern Monitor* Safepoint_lock;                  // a lock used by the safepoint abstraction
 extern Monitor* Threads_lock;                    // a lock on the Threads table of active Java threads
                                                  // (also used by Safepoints too to block threads creation/destruction)
-extern Mutex*   NamedThreadsList_lock;           // a lock on the NamedThreads list
+extern Mutex*   NonJavaThreadsList_lock;         // a lock on the NonJavaThreads list
 extern Monitor* CGC_lock;                        // used for coordination between
                                                  // fore- & background GC threads.
 extern Monitor* STS_lock;                        // used for joining/leaving SuspendibleThreadSet.
--- a/src/hotspot/share/runtime/thread.cpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/runtime/thread.cpp	Tue Aug 28 16:04:54 2018 -0400
@@ -86,6 +86,7 @@
 #include "runtime/prefetch.inline.hpp"
 #include "runtime/safepoint.hpp"
 #include "runtime/safepointMechanism.inline.hpp"
+#include "runtime/safepointVerifiers.hpp"
 #include "runtime/sharedRuntime.hpp"
 #include "runtime/statSampler.hpp"
 #include "runtime/stubRoutines.hpp"
@@ -168,13 +169,6 @@
 // Current thread is maintained as a thread-local variable
 THREAD_LOCAL_DECL Thread* Thread::_thr_current = NULL;
 #endif
-// Class hierarchy
-// - Thread
-//   - VMThread
-//   - WatcherThread
-//   - ConcurrentMarkSweepThread
-//   - JavaThread
-//     - CompilerThread
 
 // ======= Thread ========
 // Support for forcing alignment of thread objects for biased locking
@@ -1207,61 +1201,63 @@
                           THREAD);
 }
 
-// List of all NamedThreads and safe iteration over that list.
-
-class NamedThread::List {
+// List of all NonJavaThreads and safe iteration over that list.
+
+class NonJavaThread::List {
 public:
-  NamedThread* volatile _head;
+  NonJavaThread* volatile _head;
   SingleWriterSynchronizer _protect;
 
   List() : _head(NULL), _protect() {}
 };
 
-NamedThread::List NamedThread::_the_list;
-
-NamedThread::Iterator::Iterator() :
+NonJavaThread::List NonJavaThread::_the_list;
+
+NonJavaThread::Iterator::Iterator() :
   _protect_enter(_the_list._protect.enter()),
   _current(OrderAccess::load_acquire(&_the_list._head))
 {}
 
-NamedThread::Iterator::~Iterator() {
+NonJavaThread::Iterator::~Iterator() {
   _the_list._protect.exit(_protect_enter);
 }
 
-void NamedThread::Iterator::step() {
+void NonJavaThread::Iterator::step() {
   assert(!end(), "precondition");
-  _current = OrderAccess::load_acquire(&_current->_next_named_thread);
+  _current = OrderAccess::load_acquire(&_current->_next);
+}
+
+NonJavaThread::NonJavaThread() : Thread(), _next(NULL) {
+  // Add this thread to _the_list.
+  MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
+  _next = _the_list._head;
+  OrderAccess::release_store(&_the_list._head, this);
+}
+
+NonJavaThread::~NonJavaThread() {
+  // Remove this thread from _the_list.
+  MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
+  NonJavaThread* volatile* p = &_the_list._head;
+  for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) {
+    if (t == this) {
+      *p = this->_next;
+      // Wait for any in-progress iterators.
+      _the_list._protect.synchronize();
+      break;
+    }
+  }
 }
 
 // NamedThread --  non-JavaThread subclasses with multiple
 // uniquely named instances should derive from this.
 NamedThread::NamedThread() :
-  Thread(),
+  NonJavaThread(),
   _name(NULL),
   _processed_thread(NULL),
-  _gc_id(GCId::undefined()),
-  _next_named_thread(NULL)
-{
-  // Add this thread to _the_list.
-  MutexLockerEx lock(NamedThreadsList_lock, Mutex::_no_safepoint_check_flag);
-  _next_named_thread = _the_list._head;
-  OrderAccess::release_store(&_the_list._head, this);
-}
+  _gc_id(GCId::undefined())
+{}
 
 NamedThread::~NamedThread() {
-  // Remove this thread from _the_list.
-  {
-    MutexLockerEx lock(NamedThreadsList_lock, Mutex::_no_safepoint_check_flag);
-    NamedThread* volatile* p = &_the_list._head;
-    for (NamedThread* t = *p; t != NULL; p = &t->_next_named_thread, t = *p) {
-      if (t == this) {
-        *p = this->_next_named_thread;
-        // Wait for any in-progress iterators.
-        _the_list._protect.synchronize();
-        break;
-      }
-    }
-  }
   if (_name != NULL) {
     FREE_C_HEAP_ARRAY(char, _name);
     _name = NULL;
@@ -1299,7 +1295,7 @@
 bool WatcherThread::_startable = false;
 volatile bool  WatcherThread::_should_terminate = false;
 
-WatcherThread::WatcherThread() : Thread() {
+WatcherThread::WatcherThread() : NonJavaThread() {
   assert(watcher_thread() == NULL, "we can only allocate one WatcherThread");
   if (os::create_thread(this, os::watcher_thread)) {
     _watcher_thread = this;
@@ -3430,35 +3426,12 @@
 // All JavaThreads
 #define ALL_JAVA_THREADS(X) DO_JAVA_THREADS(ThreadsSMRSupport::get_java_thread_list(), X)
 
-// All non-JavaThreads (i.e., every non-JavaThread in the system).
+// All NonJavaThreads (i.e., every non-JavaThread in the system).
 void Threads::non_java_threads_do(ThreadClosure* tc) {
-  // Someday we could have a table or list of all non-JavaThreads.
-  // For now, just manually iterate through them.
-  tc->do_thread(VMThread::vm_thread());
-  if (Universe::heap() != NULL) {
-    Universe::heap()->gc_threads_do(tc);
-  }
-  WatcherThread *wt = WatcherThread::watcher_thread();
-  // Strictly speaking, the following NULL check isn't sufficient to make sure
-  // the data for WatcherThread is still valid upon being examined. However,
-  // considering that WatchThread terminates when the VM is on the way to
-  // exit at safepoint, the chance of the above is extremely small. The right
-  // way to prevent termination of WatcherThread would be to acquire
-  // Terminator_lock, but we can't do that without violating the lock rank
-  // checking in some cases.
-  if (wt != NULL) {
-    tc->do_thread(wt);
-  }
-
-#if INCLUDE_JFR
-  Thread* sampler_thread = Jfr::sampler_thread();
-  if (sampler_thread != NULL) {
-    tc->do_thread(sampler_thread);
-  }
-
-#endif
-
-  // If CompilerThreads ever become non-JavaThreads, add them here
+  NoSafepointVerifier nsv(!SafepointSynchronize::is_at_safepoint(), false);
+  for (NonJavaThread::Iterator njti; !njti.end(); njti.step()) {
+    tc->do_thread(njti.current());
+  }
 }
 
 // All JavaThreads
--- a/src/hotspot/share/runtime/thread.hpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/runtime/thread.hpp	Tue Aug 28 16:04:54 2018 -0400
@@ -93,16 +93,21 @@
 
 // Class hierarchy
 // - Thread
-//   - NamedThread
-//     - VMThread
-//     - ConcurrentGCThread
-//     - WorkerThread
-//       - GangWorker
-//       - GCTaskThread
 //   - JavaThread
 //     - various subclasses eg CompilerThread, ServiceThread
-//   - WatcherThread
-//   - JfrSamplerThread
+//   - NonJavaThread
+//     - NamedThread
+//       - VMThread
+//       - ConcurrentGCThread
+//       - WorkerThread
+//         - GangWorker
+//         - GCTaskThread
+//     - WatcherThread
+//     - JfrThreadSampler
+//
+// All Thread subclasses must be either JavaThread or NonJavaThread.
+// This means !t->is_Java_thread() iff t is a NonJavaThread, or t is
+// a partially constructed/destroyed Thread.
 
 class Thread: public ThreadShadow {
   friend class VMStructs;
@@ -380,7 +385,7 @@
 
   // Constructor
   Thread();
-  virtual ~Thread();
+  virtual ~Thread() = 0;        // Thread is abstract.
 
   // Manage Thread::current()
   void initialize_thread_current();
@@ -764,9 +769,47 @@
   return NULL;
 }
 
+class NonJavaThread: public Thread {
+  friend class VMStructs;
+
+  NonJavaThread* volatile _next;
+
+  class List;
+  static List _the_list;
+
+ public:
+  NonJavaThread();
+  ~NonJavaThread();
+
+  class Iterator;
+};
+
+// Provides iteration over the list of NonJavaThreads.  Because list
+// management occurs in the NonJavaThread constructor and destructor,
+// entries in the list may not be fully constructed instances of a
+// derived class.  Threads created after an iterator is constructed
+// will not be visited by the iterator.  The scope of an iterator is a
+// critical section; there must be no safepoint checks in that scope.
+class NonJavaThread::Iterator : public StackObj {
+  uint _protect_enter;
+  NonJavaThread* _current;
+
+  // Noncopyable.
+  Iterator(const Iterator&);
+  Iterator& operator=(const Iterator&);
+
+public:
+  Iterator();
+  ~Iterator();
+
+  bool end() const { return _current == NULL; }
+  NonJavaThread* current() const { return _current; }
+  void step();
+};
+
 // Name support for threads.  non-JavaThread subclasses with multiple
 // uniquely named instances should derive from this.
-class NamedThread: public Thread {
+class NamedThread: public NonJavaThread {
   friend class VMStructs;
   enum {
     max_name_len = 64
@@ -776,10 +819,6 @@
   // log JavaThread being processed by oops_do
   JavaThread* _processed_thread;
   uint _gc_id; // The current GC id when a thread takes part in GC
-  NamedThread* volatile _next_named_thread;
-
-  class List;
-  static List _the_list;
 
  public:
   NamedThread();
@@ -795,31 +834,6 @@
 
   void set_gc_id(uint gc_id) { _gc_id = gc_id; }
   uint gc_id() { return _gc_id; }
-
-  class Iterator;
-};
-
-// Provides iteration over the list of NamedThreads.  Because list
-// management occurs in the NamedThread constructor and destructor,
-// entries in the list may not be fully constructed instances of a
-// derived class.  Threads created after an iterator is constructed
-// will not be visited by the iterator.  The scope of an iterator is a
-// critical section; there must be no safepoint checks in that scope.
-class NamedThread::Iterator : public StackObj {
-  uint _protect_enter;
-  NamedThread* _current;
-
-  // Noncopyable.
-  Iterator(const Iterator&);
-  Iterator& operator=(const Iterator&);
-
-public:
-  Iterator();
-  ~Iterator();
-
-  bool end() const { return _current == NULL; }
-  NamedThread* current() const { return _current; }
-  void step();
 };
 
 // Worker threads are named and have an id of an assigned work.
@@ -840,7 +854,7 @@
 };
 
 // A single WatcherThread is used for simulating timer interrupts.
-class WatcherThread: public Thread {
+class WatcherThread: public NonJavaThread {
   friend class VMStructs;
  public:
   virtual void run();
--- a/src/hotspot/share/runtime/vmStructs.cpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/runtime/vmStructs.cpp	Tue Aug 28 16:04:54 2018 -0400
@@ -1355,14 +1355,15 @@
                                                                           \
   declare_toplevel_type(Threads)                                          \
   declare_toplevel_type(ThreadShadow)                                     \
-           declare_type(Thread, ThreadShadow)                             \
-           declare_type(NamedThread, Thread)                              \
-           declare_type(WatcherThread, Thread)                            \
-           declare_type(JavaThread, Thread)                               \
-           declare_type(JvmtiAgentThread, JavaThread)                     \
-           declare_type(ServiceThread, JavaThread)                        \
-  declare_type(CompilerThread, JavaThread)                                \
-  declare_type(CodeCacheSweeperThread, JavaThread)                        \
+    declare_type(Thread, ThreadShadow)                                    \
+      declare_type(NonJavaThread, Thread)                                 \
+        declare_type(NamedThread, NonJavaThread)                          \
+        declare_type(WatcherThread, NonJavaThread)                        \
+      declare_type(JavaThread, Thread)                                    \
+        declare_type(JvmtiAgentThread, JavaThread)                        \
+        declare_type(ServiceThread, JavaThread)                           \
+        declare_type(CompilerThread, JavaThread)                          \
+        declare_type(CodeCacheSweeperThread, JavaThread)                  \
   declare_toplevel_type(OSThread)                                         \
   declare_toplevel_type(JavaFrameAnchor)                                  \
                                                                           \
--- a/src/hotspot/share/utilities/globalCounter.cpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/utilities/globalCounter.cpp	Tue Aug 28 16:04:54 2018 -0400
@@ -71,7 +71,7 @@
   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thread = jtiwh.next(); ) {
     ctc.do_thread(thread);
   }
-  for (NamedThread::Iterator nti; !nti.end(); nti.step()) {
-    ctc.do_thread(nti.current());
+  for (NonJavaThread::Iterator njti; !njti.end(); njti.step()) {
+    ctc.do_thread(njti.current());
   }
 }
--- a/src/hotspot/share/utilities/globalCounter.inline.hpp	Tue Aug 28 14:45:34 2018 -0400
+++ b/src/hotspot/share/utilities/globalCounter.inline.hpp	Tue Aug 28 16:04:54 2018 -0400
@@ -31,7 +31,6 @@
 
 inline void GlobalCounter::critical_section_begin(Thread *thread) {
   assert(thread == Thread::current(), "must be current thread");
-  assert(thread->is_Named_thread() || thread->is_Java_thread(), "must be NamedThread or JavaThread");
   assert((*thread->get_rcu_counter() & COUNTER_ACTIVE) == 0x0, "nested critical sections, not supported yet");
   uintx gbl_cnt = OrderAccess::load_acquire(&_global_counter._counter);
   OrderAccess::release_store_fence(thread->get_rcu_counter(), gbl_cnt | COUNTER_ACTIVE);
@@ -39,7 +38,6 @@
 
 inline void GlobalCounter::critical_section_end(Thread *thread) {
   assert(thread == Thread::current(), "must be current thread");
-  assert(thread->is_Named_thread() || thread->is_Java_thread(), "must be NamedThread or JavaThread");
   assert((*thread->get_rcu_counter() & COUNTER_ACTIVE) == COUNTER_ACTIVE, "must be in critical section");
   // Mainly for debugging we set it to 'now'.
   uintx gbl_cnt = OrderAccess::load_acquire(&_global_counter._counter);