8193559: ugly DO_JAVA_THREADS macro should be replaced

Co-authored-by: Kim Barrett <kbarrett@openjdk.org>
Reviewed-by: eosterlund, ayang, kbarrett, dholmes

src/hotspot/share/runtime/thread.cpp

@@ -94,7 +94,6 @@
 #include "runtime/objectMonitor.hpp"
 #include "runtime/orderAccess.hpp"
 #include "runtime/osThread.hpp"
-#include "runtime/prefetch.inline.hpp"
 #include "runtime/safepoint.hpp"
 #include "runtime/safepointMechanism.inline.hpp"
 #include "runtime/safepointVerifiers.hpp"
@@ -2478,28 +2477,6 @@ size_t      JavaThread::_stack_size_at_create = 0;
 bool        Threads::_vm_complete = false;
 #endif
 
-static inline void *prefetch_and_load_ptr(void **addr, intx prefetch_interval) {
-  Prefetch::read((void*)addr, prefetch_interval);
-  return *addr;
-}
-
-// Possibly the ugliest for loop the world has seen. C++ does not allow
-// multiple types in the declaration section of the for loop. In this case
-// we are only dealing with pointers and hence can cast them. It looks ugly
-// but macros are ugly and therefore it's fine to make things absurdly ugly.
-#define DO_JAVA_THREADS(LIST, X)                                                                                          \
-    for (JavaThread *MACRO_scan_interval = (JavaThread*)(uintptr_t)PrefetchScanIntervalInBytes,                           \
-             *MACRO_list = (JavaThread*)(LIST),                                                                           \
-             **MACRO_end = ((JavaThread**)((ThreadsList*)MACRO_list)->threads()) + ((ThreadsList*)MACRO_list)->length(),  \
-             **MACRO_current_p = (JavaThread**)((ThreadsList*)MACRO_list)->threads(),                                     \
-             *X = (JavaThread*)prefetch_and_load_ptr((void**)MACRO_current_p, (intx)MACRO_scan_interval);                 \
-         MACRO_current_p != MACRO_end;                                                                                    \
-         MACRO_current_p++,                                                                                               \
-             X = (JavaThread*)prefetch_and_load_ptr((void**)MACRO_current_p, (intx)MACRO_scan_interval))
-
-// All JavaThreads
-#define ALL_JAVA_THREADS(X) DO_JAVA_THREADS(ThreadsSMRSupport::get_java_thread_list(), X)
-
 // All NonJavaThreads (i.e., every non-JavaThread in the system).
 void Threads::non_java_threads_do(ThreadClosure* tc) {
   NoSafepointVerifier nsv;
@@ -2508,6 +2485,10 @@ void Threads::non_java_threads_do(ThreadClosure* tc) {
   }
 }
 
+// All JavaThreads
+#define ALL_JAVA_THREADS(X) \
+  for (JavaThread* X : *ThreadsSMRSupport::get_java_thread_list())
+
 // All JavaThreads
 void Threads::java_threads_do(ThreadClosure* tc) {
   assert_locked_or_safepoint(Threads_lock);
@@ -3641,7 +3622,7 @@ GrowableArray<JavaThread*>* Threads::get_pending_threads(ThreadsList * t_list,
   GrowableArray<JavaThread*>* result = new GrowableArray<JavaThread*>(count);
 
   int i = 0;
-  DO_JAVA_THREADS(t_list, p) {
+  for (JavaThread* p : *t_list) {
     if (!p->can_call_java()) continue;
 
     // The first stage of async deflation does not affect any field
@@ -3662,7 +3643,7 @@ JavaThread *Threads::owning_thread_from_monitor_owner(ThreadsList * t_list,
   // NULL owner means not locked so we can skip the search
   if (owner == NULL) return NULL;
 
-  DO_JAVA_THREADS(t_list, p) {
+  for (JavaThread* p : *t_list) {
     // first, see if owner is the address of a Java thread
     if (owner == (address)p) return p;
   }
@@ -3677,7 +3658,7 @@ JavaThread *Threads::owning_thread_from_monitor_owner(ThreadsList * t_list,
   // Lock Word in the owning Java thread's stack.
   //
   JavaThread* the_owner = NULL;
-  DO_JAVA_THREADS(t_list, q) {
+  for (JavaThread* q : *t_list) {
     if (q->is_lock_owned(owner)) {
       the_owner = q;
       break;

src/hotspot/share/runtime/threadSMR.cpp

@@ -35,6 +35,7 @@
 #include "services/threadIdTable.hpp"
 #include "services/threadService.hpp"
 #include "utilities/copy.hpp"
+#include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/ostream.hpp"
 #include "utilities/powerOfTwo.hpp"
@@ -629,6 +630,29 @@ static JavaThread* const* make_threads_list_data(int entries) {
   return data;
 }
 
+#ifdef ASSERT
+
+ThreadsList::Iterator::Iterator() : _thread_ptr(nullptr), _list(nullptr) {}
+
+uint ThreadsList::Iterator::check_index(ThreadsList* list, uint i) {
+  assert(i <= list->length(), "invalid index %u", i);
+  return i;
+}
+
+void ThreadsList::Iterator::assert_not_singular() const {
+  assert(_list != nullptr, "singular iterator");
+}
+
+void ThreadsList::Iterator::assert_dereferenceable() const {
+  assert(_thread_ptr < (_list->threads() + _list->length()), "not dereferenceable");
+}
+
+void ThreadsList::Iterator::assert_same_list(Iterator i) const {
+  assert(_list == i._list, "iterators from different lists");
+}
+
+#endif // ASSERT
+
 ThreadsList::ThreadsList(int entries) :
   _magic(THREADS_LIST_MAGIC),
   _length(entries),

src/hotspot/share/runtime/threadSMR.hpp

@@ -29,6 +29,7 @@
 #include "runtime/mutexLocker.hpp"
 #include "runtime/thread.hpp"
 #include "runtime/timer.hpp"
+#include "utilities/debug.hpp"
 
 class JavaThread;
 class Monitor;
@@ -192,6 +193,10 @@ class ThreadsList : public CHeapObj<mtThread> {
   explicit ThreadsList(int entries);
   ~ThreadsList();
 
+  class Iterator;
+  inline Iterator begin();
+  inline Iterator end();
+
   template <class T>
   void threads_do(T *cl) const;
 
@@ -211,6 +216,29 @@ class ThreadsList : public CHeapObj<mtThread> {
 #endif
 };
 
+class ThreadsList::Iterator {
+  JavaThread* const* _thread_ptr;
+  DEBUG_ONLY(ThreadsList* _list;)
+
+  static uint check_index(ThreadsList* list, uint i) NOT_DEBUG({ return i; });
+  void assert_not_singular() const NOT_DEBUG_RETURN;
+  void assert_dereferenceable() const NOT_DEBUG_RETURN;
+  void assert_same_list(Iterator i) const NOT_DEBUG_RETURN;
+
+public:
+  Iterator() NOT_DEBUG(= default); // Singular iterator.
+  inline Iterator(ThreadsList* list, uint i);
+
+  inline bool operator==(Iterator other) const;
+  inline bool operator!=(Iterator other) const;
+
+  inline JavaThread* operator*() const;
+  inline JavaThread* operator->() const;
+
+  inline Iterator& operator++();
+  inline Iterator operator++(int);
+};
+
 // An abstract safe ptr to a ThreadsList comprising either a stable hazard ptr
 // for leaves, or a retained reference count for nested uses. The user of this
 // API does not need to know which mechanism is providing the safety.

src/hotspot/share/runtime/threadSMR.inline.hpp

@@ -33,6 +33,57 @@
 #include "memory/iterator.hpp"
 #include "runtime/prefetch.inline.hpp"
 #include "runtime/thread.inline.hpp"
+#include "utilities/debug.hpp"
+#include "utilities/macros.hpp"
+
+ThreadsList::Iterator::Iterator(ThreadsList* list, uint i) :
+  _thread_ptr(list->threads() + check_index(list, i))
+  DEBUG_ONLY(COMMA _list(list))
+{}
+
+bool ThreadsList::Iterator::operator==(Iterator i) const {
+  assert_not_singular();
+  assert_same_list(i);
+  return _thread_ptr == i._thread_ptr;
+}
+
+bool ThreadsList::Iterator::operator!=(Iterator i) const {
+  return !operator==(i);
+}
+
+JavaThread* ThreadsList::Iterator::operator*() const {
+  assert_not_singular();
+  assert_dereferenceable();
+  Prefetch::read(const_cast<JavaThread**>(_thread_ptr), PrefetchScanIntervalInBytes);
+  return *_thread_ptr;
+}
+
+JavaThread* ThreadsList::Iterator::operator->() const {
+  return operator*();
+}
+
+ThreadsList::Iterator& ThreadsList::Iterator::operator++() {
+  assert_not_singular();
+  assert_dereferenceable();
+  ++_thread_ptr;
+  return *this;
+}
+
+ThreadsList::Iterator ThreadsList::Iterator::operator++(int) {
+  assert_not_singular();
+  assert_dereferenceable();
+  Iterator result = *this;
+  ++_thread_ptr;
+  return result;
+}
+
+ThreadsList::Iterator ThreadsList::begin() {
+  return Iterator(this, 0);
+}
+
+ThreadsList::Iterator ThreadsList::end() {
+  return Iterator(this, length());
+}
 
 // Devirtualize known thread closure types.
 template <class T>