8209976: Improve iteration over non-JavaThreads

Add NonJavaThread and move NamedThread iteration to new class.

Reviewed-by: eosterlund, coleenp, rkennke

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp

@@ -308,7 +308,7 @@ JfrThreadSampleClosure::JfrThreadSampleClosure(EventExecutionSample* events, Eve
   _added_native(0) {
 }
 
-class JfrThreadSampler : public Thread {
+class JfrThreadSampler : public NonJavaThread {
   friend class JfrThreadSampling;
  private:
   Semaphore _sample;

src/hotspot/share/runtime/mutexLocker.cpp

@@ -76,7 +76,7 @@ Monitor* VMOperationRequest_lock      = NULL;
 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 @@ void mutex_init() {
   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);

src/hotspot/share/runtime/mutexLocker.hpp

@@ -72,7 +72,7 @@ extern Monitor* VMOperationRequest_lock;         // a lock on Threads waiting fo
 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.

src/hotspot/share/runtime/thread.cpp

@@ -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 @@ void universe_post_module_init();  // must happen after call_initPhase2
 // 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 @@ void JavaThread::allocate_threadObj(Handle thread_group, const char* thread_name
                           THREAD);
 }
 
-// List of all NamedThreads and safe iteration over that list.
+// List of all NonJavaThreads and safe iteration over that list.
 
-class NamedThread::List {
+class NonJavaThread::List {
 public:
-  NamedThread* volatile _head;
+  NonJavaThread* volatile _head;
   SingleWriterSynchronizer _protect;
 
   List() : _head(NULL), _protect() {}
 };
 
-NamedThread::List NamedThread::_the_list;
+NonJavaThread::List NonJavaThread::_the_list;
 
-NamedThread::Iterator::Iterator() :
+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);
 }
 
-// NamedThread --  non-JavaThread subclasses with multiple
+NonJavaThread::NonJavaThread() : Thread(), _next(NULL) {
-// uniquely named instances should derive from this.
-NamedThread::NamedThread() :
-  Thread(),
-  _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);
+  MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
-  _next_named_thread = _the_list._head;
+  _next = _the_list._head;
   OrderAccess::release_store(&_the_list._head, this);
 }
 
-NamedThread::~NamedThread() {
+NonJavaThread::~NonJavaThread() {
   // Remove this thread from _the_list.
-  {
+  MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
-    MutexLockerEx lock(NamedThreadsList_lock, Mutex::_no_safepoint_check_flag);
+  NonJavaThread* volatile* p = &_the_list._head;
-    NamedThread* volatile* p = &_the_list._head;
+  for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) {
-    for (NamedThread* t = *p; t != NULL; p = &t->_next_named_thread, t = *p) {
     if (t == this) {
-        *p = this->_next_named_thread;
+      *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() :
+  NonJavaThread(),
+  _name(NULL),
+  _processed_thread(NULL),
+  _gc_id(GCId::undefined())
+{}
+
+NamedThread::~NamedThread() {
   if (_name != NULL) {
     FREE_C_HEAP_ARRAY(char, _name);
     _name = NULL;
@@ -1299,7 +1295,7 @@ WatcherThread* WatcherThread::_watcher_thread   = NULL;
 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 @@ static inline void *prefetch_and_load_ptr(void **addr, intx prefetch_interval) {
 // 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.
+  NoSafepointVerifier nsv(!SafepointSynchronize::is_at_safepoint(), false);
-  // For now, just manually iterate through them.
+  for (NonJavaThread::Iterator njti; !njti.end(); njti.step()) {
-  tc->do_thread(VMThread::vm_thread());
+    tc->do_thread(njti.current());
-  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
 }
 
 // All JavaThreads

src/hotspot/share/runtime/thread.hpp

@@ -93,16 +93,21 @@ class WorkerThread;
 
 // Class hierarchy
 // - Thread
+//   - JavaThread
+//     - various subclasses eg CompilerThread, ServiceThread
+//   - NonJavaThread
 //     - NamedThread
 //       - VMThread
 //       - ConcurrentGCThread
 //       - WorkerThread
 //         - GangWorker
 //         - GCTaskThread
-//   - JavaThread
-//     - various subclasses eg CompilerThread, ServiceThread
 //     - WatcherThread
-//   - JfrSamplerThread
+//     - 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 @@ class Thread: public ThreadShadow {
 
   // Constructor
   Thread();
-  virtual ~Thread();
+  virtual ~Thread() = 0;        // Thread is abstract.
 
   // Manage Thread::current()
   void initialize_thread_current();
@@ -764,9 +769,47 @@ inline Thread* Thread::current_or_null_safe() {
   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 @@ class NamedThread: public Thread {
   // 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 @@ class NamedThread: public Thread {
 
   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 @@ class WorkerThread: public NamedThread {
 };
 
 // A single WatcherThread is used for simulating timer interrupts.
-class WatcherThread: public Thread {
+class WatcherThread: public NonJavaThread {
   friend class VMStructs;
  public:
   virtual void run();

src/hotspot/share/runtime/vmStructs.cpp

@@ -1356,8 +1356,9 @@ typedef PaddedEnd<ObjectMonitor>              PaddedObjectMonitor;
   declare_toplevel_type(Threads)                                          \
   declare_toplevel_type(ThreadShadow)                                     \
     declare_type(Thread, ThreadShadow)                                    \
-           declare_type(NamedThread, Thread)                              \
+      declare_type(NonJavaThread, Thread)                                 \
-           declare_type(WatcherThread, Thread)                            \
+        declare_type(NamedThread, NonJavaThread)                          \
+        declare_type(WatcherThread, NonJavaThread)                        \
       declare_type(JavaThread, Thread)                                    \
         declare_type(JvmtiAgentThread, JavaThread)                        \
         declare_type(ServiceThread, JavaThread)                           \

src/hotspot/share/utilities/globalCounter.cpp

@@ -71,7 +71,7 @@ void GlobalCounter::write_synchronize() {
   for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thread = jtiwh.next(); ) {
     ctc.do_thread(thread);
   }
-  for (NamedThread::Iterator nti; !nti.end(); nti.step()) {
+  for (NonJavaThread::Iterator njti; !njti.end(); njti.step()) {
-    ctc.do_thread(nti.current());
+    ctc.do_thread(njti.current());
   }
 }

src/hotspot/share/utilities/globalCounter.inline.hpp

@@ -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_begin(Thread *thread) {
 
 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);