8363932: G1: Better distribute KlassCleaningTask

Reviewed-by: ayang, coleenp

src/hotspot/share/classfile/classLoaderData.hpp

@@ -97,7 +97,7 @@ class ClassLoaderData : public CHeapObj<mtClass> {
   };
 
   friend class ClassLoaderDataGraph;
-  friend class ClassLoaderDataGraphKlassIteratorAtomic;
+  friend class ClassLoaderDataGraphIteratorAtomic;
   friend class Klass;
   friend class MetaDataFactory;
   friend class Method;

src/hotspot/share/classfile/classLoaderDataGraph.cpp

@@ -489,62 +489,25 @@ void ClassLoaderDataGraph::purge(bool at_safepoint) {
   }
 }
 
-ClassLoaderDataGraphKlassIteratorAtomic::ClassLoaderDataGraphKlassIteratorAtomic()
-    : _next_klass(nullptr) {
+ClassLoaderDataGraphIteratorAtomic::ClassLoaderDataGraphIteratorAtomic()
+    : _cld(nullptr) {
   assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint!");
-  ClassLoaderData* cld = ClassLoaderDataGraph::_head;
-  Klass* klass = nullptr;
-
-  // Find the first klass in the CLDG.
-  while (cld != nullptr) {
-    assert_locked_or_safepoint(cld->metaspace_lock());
-    klass = cld->_klasses;
-    if (klass != nullptr) {
-      _next_klass = klass;
-      return;
-    }
-    cld = cld->next();
-  }
+  _cld = AtomicAccess::load_acquire(&ClassLoaderDataGraph::_head);
 }
 
-Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass_in_cldg(Klass* klass) {
-  Klass* next = klass->next_link();
-  if (next != nullptr) {
-    return next;
-  }
-
-  // No more klasses in the current CLD. Time to find a new CLD.
-  ClassLoaderData* cld = klass->class_loader_data();
-  assert_locked_or_safepoint(cld->metaspace_lock());
-  while (next == nullptr) {
-    cld = cld->next();
-    if (cld == nullptr) {
-      break;
+ClassLoaderData* ClassLoaderDataGraphIteratorAtomic::next() {
+  ClassLoaderData* cur = AtomicAccess::load(&_cld);
+  for (;;) {
+    if (cur == nullptr) {
+      return nullptr;
     }
-    next = cld->_klasses;
-  }
-
-  return next;
-}
-
-Klass* ClassLoaderDataGraphKlassIteratorAtomic::next_klass() {
-  Klass* head = _next_klass;
-
-  while (head != nullptr) {
-    Klass* next = next_klass_in_cldg(head);
-
-    Klass* old_head = AtomicAccess::cmpxchg(&_next_klass, head, next);
-
-    if (old_head == head) {
-      return head; // Won the CAS.
+    ClassLoaderData* next = cur->next();
+    ClassLoaderData* old;
+    if ((old = AtomicAccess::cmpxchg(&_cld, cur, next)) == cur) {
+      return cur;
     }
-
-    head = old_head;
+    cur = old;
   }
-
-  // Nothing more for the iterator to hand out.
-  assert(head == nullptr, "head is " PTR_FORMAT ", expected not null:", p2i(head));
-  return nullptr;
 }
 
 void ClassLoaderDataGraph::verify() {

src/hotspot/share/classfile/classLoaderDataGraph.hpp

@@ -34,7 +34,7 @@
 
 class ClassLoaderDataGraph : public AllStatic {
   friend class ClassLoaderData;
-  friend class ClassLoaderDataGraphKlassIteratorAtomic;
+  friend class ClassLoaderDataGraphIteratorAtomic;
   friend class VMStructs;
  private:
   class ClassLoaderDataGraphIterator;
@@ -140,14 +140,14 @@ public:
   }
 };
 
-// An iterator that distributes Klasses to parallel worker threads.
-class ClassLoaderDataGraphKlassIteratorAtomic : public StackObj {
- Klass* volatile _next_klass;
- public:
-  ClassLoaderDataGraphKlassIteratorAtomic();
-  Klass* next_klass();
- private:
-  static Klass* next_klass_in_cldg(Klass* klass);
+// An iterator that distributes Klasses to parallel worker threads based on CLDs.
+class ClassLoaderDataGraphIteratorAtomic : public StackObj {
+  ClassLoaderData* volatile _cld;
+
+public:
+  ClassLoaderDataGraphIteratorAtomic();
+
+  ClassLoaderData* next();
 };
 
 #endif // SHARE_CLASSFILE_CLASSLOADERDATAGRAPH_HPP

src/hotspot/share/gc/shared/parallelCleaning.cpp

@@ -27,7 +27,7 @@
 #include "code/codeCache.hpp"
 #include "gc/shared/parallelCleaning.hpp"
 #include "logging/log.hpp"
-#include "memory/resourceArea.hpp"
+#include "oops/klass.inline.hpp"
 #include "runtime/atomicAccess.hpp"
 
 CodeCacheUnloadingTask::CodeCacheUnloadingTask(uint num_workers, bool unloading_occurred) :
@@ -94,38 +94,26 @@ void CodeCacheUnloadingTask::work(uint worker_id) {
   }
 }
 
-KlassCleaningTask::KlassCleaningTask() :
-  _clean_klass_tree_claimed(false),
-  _klass_iterator() {
-}
-
-bool KlassCleaningTask::claim_clean_klass_tree_task() {
-  if (_clean_klass_tree_claimed) {
-    return false;
-  }
-
-  return !AtomicAccess::cmpxchg(&_clean_klass_tree_claimed, false, true);
-}
-
-InstanceKlass* KlassCleaningTask::claim_next_klass() {
-  Klass* klass;
-  do {
-    klass =_klass_iterator.next_klass();
-  } while (klass != nullptr && !klass->is_instance_klass());
-
-  // this can be null so don't call InstanceKlass::cast
-  return static_cast<InstanceKlass*>(klass);
-}
-
 void KlassCleaningTask::work() {
-  // One worker will clean the subklass/sibling klass tree.
-  if (claim_clean_klass_tree_task()) {
-    Klass::clean_weak_klass_links(true /* class_unloading_occurred */, false /* clean_alive_klasses */);
-  }
+  for (ClassLoaderData* cur = _cld_iterator_atomic.next(); cur != nullptr; cur = _cld_iterator_atomic.next()) {
+      class CleanKlasses : public KlassClosure {
+      public:
 
-  // All workers will help cleaning the classes,
-  InstanceKlass* klass;
-  while ((klass = claim_next_klass()) != nullptr) {
-    Klass::clean_weak_instanceklass_links(klass);
+        void do_klass(Klass* klass) override {
+          klass->clean_subklass(true);
+
+          Klass* sibling = klass->next_sibling(true);
+          klass->set_next_sibling(sibling);
+
+          if (klass->is_instance_klass()) {
+            Klass::clean_weak_instanceklass_links(InstanceKlass::cast(klass));
+          }
+
+          assert(klass->subklass() == nullptr || klass->subklass()->is_loader_alive(), "must be");
+          assert(klass->next_sibling(false) == nullptr || klass->next_sibling(false)->is_loader_alive(), "must be");
+        }
+      } cl;
+
+      cur->classes_do(&cl);
   }
 }

src/hotspot/share/gc/shared/parallelCleaning.hpp

@@ -54,14 +54,10 @@ public:
 
 // Cleans out the Klass tree from stale data.
 class KlassCleaningTask : public StackObj {
-  volatile bool _clean_klass_tree_claimed;
-  ClassLoaderDataGraphKlassIteratorAtomic _klass_iterator;
-
-  bool claim_clean_klass_tree_task();
-  InstanceKlass* claim_next_klass();
+  ClassLoaderDataGraphIteratorAtomic _cld_iterator_atomic;
 
 public:
-  KlassCleaningTask();
+  KlassCleaningTask() : _cld_iterator_atomic() { }
 
   void work();
 };

src/hotspot/share/oops/klass.cpp

@@ -614,8 +614,7 @@ GrowableArray<Klass*>* Klass::compute_secondary_supers(int num_extra_slots,
 
 // subklass links.  Used by the compiler (and vtable initialization)
 // May be cleaned concurrently, so must use the Compile_lock.
-// The log parameter is for clean_weak_klass_links to report unlinked classes.
-Klass* Klass::subklass(bool log) const {
+Klass* Klass::subklass() const {
   // Need load_acquire on the _subklass, because it races with inserts that
   // publishes freshly initialized data.
   for (Klass* chain = AtomicAccess::load_acquire(&_subklass);
@@ -626,11 +625,6 @@ Klass* Klass::subklass(bool log) const {
   {
     if (chain->is_loader_alive()) {
       return chain;
-    } else if (log) {
-      if (log_is_enabled(Trace, class, unload)) {
-        ResourceMark rm;
-        log_trace(class, unload)("unlinking class (subclass): %s", chain->external_name());
-      }
     }
   }
   return nullptr;
@@ -701,15 +695,20 @@ void Klass::append_to_sibling_list() {
   DEBUG_ONLY(verify();)
 }
 
-void Klass::clean_subklass() {
+// The log parameter is for clean_weak_klass_links to report unlinked classes.
+Klass* Klass::clean_subklass(bool log) {
   for (;;) {
     // Need load_acquire, due to contending with concurrent inserts
     Klass* subklass = AtomicAccess::load_acquire(&_subklass);
     if (subklass == nullptr || subklass->is_loader_alive()) {
-      return;
+      return subklass;
+    }
+    if (log && log_is_enabled(Trace, class, unload)) {
+      ResourceMark rm;
+      log_trace(class, unload)("unlinking class (subclass): %s", subklass->external_name());
     }
     // Try to fix _subklass until it points at something not dead.
-    AtomicAccess::cmpxchg(&_subklass, subklass, subklass->next_sibling());
+    AtomicAccess::cmpxchg(&_subklass, subklass, subklass->next_sibling(log));
   }
 }
 
@@ -728,8 +727,7 @@ void Klass::clean_weak_klass_links(bool unloading_occurred, bool clean_alive_kla
     assert(current->is_loader_alive(), "just checking, this should be live");
 
     // Find and set the first alive subklass
-    Klass* sub = current->subklass(true);
-    current->clean_subklass();
+    Klass* sub = current->clean_subklass(true);
     if (sub != nullptr) {
       stack.push(sub);
     }

src/hotspot/share/oops/klass.hpp

@@ -300,7 +300,7 @@ protected:
   // Use InstanceKlass::contains_field_offset to classify field offsets.
 
   // sub/superklass links
-  Klass* subklass(bool log = false) const;
+  Klass* subklass() const;
   Klass* next_sibling(bool log = false) const;
 
   void append_to_sibling_list();           // add newly created receiver to superklass' subklass list
@@ -413,9 +413,9 @@ protected:
   virtual ModuleEntry* module() const = 0;
   virtual PackageEntry* package() const = 0;
 
+  void     set_next_sibling(Klass* s);
  protected:                                // internal accessors
   void     set_subklass(Klass* s);
-  void     set_next_sibling(Klass* s);
 
  private:
   static uint8_t compute_hash_slot(Symbol* s);
@@ -743,7 +743,7 @@ public:
   inline bool is_loader_alive() const;
   inline bool is_loader_present_and_alive() const;
 
-  void clean_subklass();
+  Klass* clean_subklass(bool log = false);
 
   // Clean out unnecessary weak klass links from the whole klass hierarchy.
   static void clean_weak_klass_links(bool unloading_occurred, bool clean_alive_klasses = true);