8159984: Remove call to ClassLoaderDataGraph::clear_claimed_marks during the initial mark pause

The CLDG is only iterated once during garbage collection, so we do not need to claim CLDs any more.

Reviewed-by: sjohanss, kbarrett

src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp

@@ -62,8 +62,7 @@ G1GCPhaseTimes::G1GCPhaseTimes(STWGCTimer* gc_timer, uint max_gc_threads) :
   _gc_par_phases[JVMTIRoots] = new WorkerDataArray<double>(max_gc_threads, "JVMTI Roots (ms):");
   AOT_ONLY(_gc_par_phases[AOTCodeRoots] = new WorkerDataArray<double>(max_gc_threads, "AOT Root Scan (ms):");)
   _gc_par_phases[CMRefRoots] = new WorkerDataArray<double>(max_gc_threads, "CM RefProcessor Roots (ms):");
-  _gc_par_phases[WaitForStrongCLD] = new WorkerDataArray<double>(max_gc_threads, "Wait For Strong CLD (ms):");
+  _gc_par_phases[WaitForStrongRoots] = new WorkerDataArray<double>(max_gc_threads, "Wait For Strong Roots (ms):");
-  _gc_par_phases[WeakCLDRoots] = new WorkerDataArray<double>(max_gc_threads, "Weak CLD Roots (ms):");
 
   _gc_par_phases[MergeER] = new WorkerDataArray<double>(max_gc_threads, "Eager Reclaim (ms):");
 
@@ -567,8 +566,7 @@ const char* G1GCPhaseTimes::phase_name(GCParPhases phase) {
       "JVMTIRoots",
       AOT_ONLY("AOTCodeRoots" COMMA)
       "CMRefRoots",
-      "WaitForStrongCLD",
+      "WaitForStrongRoots",
-      "WeakCLDRoots",
       "MergeER",
       "MergeRS",
       "OptMergeRS",

src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp

@@ -57,8 +57,7 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
     JVMTIRoots,
     AOT_ONLY(AOTCodeRoots COMMA)
     CMRefRoots,
-    WaitForStrongCLD,
+    WaitForStrongRoots,
-    WeakCLDRoots,
     MergeER,
     MergeRS,
     OptMergeRS,
@@ -84,7 +83,7 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
   };
 
   static const GCParPhases ExtRootScanSubPhasesFirst = ThreadRoots;
-  static const GCParPhases ExtRootScanSubPhasesLast = WeakCLDRoots;
+  static const GCParPhases ExtRootScanSubPhasesLast = WaitForStrongRoots;
 
   enum GCMergeRSWorkTimes {
     MergeRSMergedSparse,

src/hotspot/share/gc/g1/g1OopClosures.cpp

@@ -43,16 +43,14 @@ G1ScanClosureBase::G1ScanClosureBase(G1CollectedHeap* g1h, G1ParScanThreadState*
 
 void G1CLDScanClosure::do_cld(ClassLoaderData* cld) {
   // If the class loader data has not been dirtied we know that there's
-  // no references into  the young gen and we can skip it.
+  // no references into the young gen and we can skip it.
   if (!_process_only_dirty || cld->has_modified_oops()) {
 
     // Tell the closure that this class loader data is the CLD to scavenge
     // and is the one to dirty if oops are left pointing into the young gen.
     _closure->set_scanned_cld(cld);
-
+    // Clean modified oops since we're going to scavenge all the metadata.
-    // Clean the cld since we're going to scavenge all the metadata.
+    cld->oops_do(_closure, ClassLoaderData::_claim_none, true /*clear_modified_oops*/);
-    // Clear modified oops only if this cld is claimed.
-    cld->oops_do(_closure, _claim, /*clear_modified_oops*/true);
 
     _closure->set_scanned_cld(NULL);
 

src/hotspot/share/gc/g1/g1OopClosures.hpp

@@ -175,12 +175,10 @@ public:
 class G1CLDScanClosure : public CLDClosure {
   G1ParCopyHelper* _closure;
   bool             _process_only_dirty;
-  int              _claim;
   int              _count;
 public:
-  G1CLDScanClosure(G1ParCopyHelper* closure,
+  G1CLDScanClosure(G1ParCopyHelper* closure, bool process_only_dirty)
-                   bool process_only_dirty, int claim_value)
+  : _closure(closure), _process_only_dirty(process_only_dirty), _count(0) {}
-  : _closure(closure), _process_only_dirty(process_only_dirty), _claim(claim_value), _count(0) {}
   void do_cld(ClassLoaderData* cld);
 };
 

src/hotspot/share/gc/g1/g1RootClosures.cpp

@@ -35,14 +35,13 @@ public:
   G1EvacuationClosures(G1CollectedHeap* g1h,
                        G1ParScanThreadState* pss,
                        bool in_young_gc) :
-      _closures(g1h, pss, in_young_gc, /* cld_claim */ ClassLoaderData::_claim_none) {}
+      _closures(g1h, pss, in_young_gc) {}
 
   OopClosure* weak_oops()   { return &_closures._oops; }
   OopClosure* strong_oops() { return &_closures._oops; }
 
   CLDClosure* weak_clds()             { return &_closures._clds; }
   CLDClosure* strong_clds()           { return &_closures._clds; }
-  CLDClosure* second_pass_weak_clds() { return NULL; }
 
   CodeBlobClosure* strong_codeblobs()      { return &_closures._codeblobs; }
   CodeBlobClosure* weak_codeblobs()        { return &_closures._codeblobs; }
@@ -58,33 +57,18 @@ class G1InitialMarkClosures : public G1EvacuationRootClosures {
   G1SharedClosures<G1MarkFromRoot> _strong;
   G1SharedClosures<MarkWeak>       _weak;
 
-  // Filter method to help with returning the appropriate closures
-  // depending on the class template parameter.
-  template <G1Mark Mark, typename T>
-  T* null_if(T* t) {
-    if (Mark == MarkWeak) {
-      return NULL;
-    }
-    return t;
-  }
-
 public:
   G1InitialMarkClosures(G1CollectedHeap* g1h,
                         G1ParScanThreadState* pss) :
-      _strong(g1h, pss, /* process_only_dirty_klasses */ false, /* cld_claim */ ClassLoaderData::_claim_strong),
+      _strong(g1h, pss, /* process_only_dirty_klasses */ false),
-      _weak(g1h, pss,   /* process_only_dirty_klasses */ false, /* cld_claim */ ClassLoaderData::_claim_strong) {}
+      _weak(g1h, pss,   /* process_only_dirty_klasses */ false) {}
 
   OopClosure* weak_oops()   { return &_weak._oops; }
   OopClosure* strong_oops() { return &_strong._oops; }
 
-  // If MarkWeak is G1MarkPromotedFromRoot then the weak CLDs must be processed in a second pass.
+  CLDClosure* weak_clds()             { return &_weak._clds; }
-  CLDClosure* weak_clds()             { return null_if<G1MarkPromotedFromRoot>(&_weak._clds); }
   CLDClosure* strong_clds()           { return &_strong._clds; }
 
-  // If MarkWeak is G1MarkFromRoot then all CLDs are processed by the weak and strong variants
-  // return a NULL closure for the following specialized versions in that case.
-  CLDClosure* second_pass_weak_clds() { return null_if<G1MarkFromRoot>(&_weak._clds); }
-
   CodeBlobClosure* strong_codeblobs()      { return &_strong._codeblobs; }
   CodeBlobClosure* weak_codeblobs()        { return &_weak._codeblobs; }
 

src/hotspot/share/gc/g1/g1RootClosures.hpp

@@ -49,10 +49,6 @@ public:
 
 class G1EvacuationRootClosures : public G1RootClosures {
 public:
-  // Applied to the weakly reachable CLDs when all strongly reachable
-  // CLDs are guaranteed to have been processed.
-  virtual CLDClosure* second_pass_weak_clds() = 0;
-
   // Applied to code blobs treated as weak roots.
   virtual CodeBlobClosure* weak_codeblobs() = 0;
 

src/hotspot/share/gc/g1/g1RootProcessor.cpp

@@ -45,7 +45,7 @@
 #include "services/management.hpp"
 #include "utilities/macros.hpp"
 
-void G1RootProcessor::worker_has_discovered_all_strong_classes() {
+void G1RootProcessor::worker_has_discovered_all_strong_nmethods() {
   assert(ClassUnloadingWithConcurrentMark, "Currently only needed when doing G1 Class Unloading");
 
   uint new_value = (uint)Atomic::add(1, &_n_workers_discovered_strong_classes);
@@ -56,7 +56,7 @@ void G1RootProcessor::worker_has_discovered_all_strong_classes() {
   }
 }
 
-void G1RootProcessor::wait_until_all_strong_classes_discovered() {
+void G1RootProcessor::wait_until_all_strong_nmethods_discovered() {
   assert(ClassUnloadingWithConcurrentMark, "Currently only needed when doing G1 Class Unloading");
 
   if ((uint)_n_workers_discovered_strong_classes != n_workers()) {
@@ -71,7 +71,7 @@ G1RootProcessor::G1RootProcessor(G1CollectedHeap* g1h, uint n_workers) :
     _g1h(g1h),
     _process_strong_tasks(G1RP_PS_NumElements),
     _srs(n_workers),
-    _lock(Mutex::leaf, "G1 Root Scanning barrier lock", false, Monitor::_safepoint_check_never),
+    _lock(Mutex::leaf, "G1 Root Scan barrier lock", false, Monitor::_safepoint_check_never),
     _n_workers_discovered_strong_classes(0) {}
 
 void G1RootProcessor::evacuate_roots(G1ParScanThreadState* pss, uint worker_i) {
@@ -80,13 +80,7 @@ void G1RootProcessor::evacuate_roots(G1ParScanThreadState* pss, uint worker_i) {
   G1EvacPhaseTimesTracker timer(phase_times, pss, G1GCPhaseTimes::ExtRootScan, worker_i);
 
   G1EvacuationRootClosures* closures = pss->closures();
-  process_java_roots(closures, phase_times, worker_i);
+  process_java_roots(closures, phase_times, worker_i, closures->trace_metadata() /* notify_claimed_nmethods_done */);
-
-  // This is the point where this worker thread will not find more strong CLDs/nmethods.
-  // Report this so G1 can synchronize the strong and weak CLDs/nmethods processing.
-  if (closures->trace_metadata()) {
-    worker_has_discovered_all_strong_classes();
-  }
 
   process_vm_roots(closures, phase_times, worker_i);
 
@@ -103,21 +97,9 @@ void G1RootProcessor::evacuate_roots(G1ParScanThreadState* pss, uint worker_i) {
   }
 
   if (closures->trace_metadata()) {
-    {
+    G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::WaitForStrongRoots, worker_i);
-      G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::WaitForStrongCLD, worker_i);
+    // Wait to make sure all workers passed the strong nmethods phase.
-      // Barrier to make sure all workers passed
+    wait_until_all_strong_nmethods_discovered();
-      // the strong CLD and strong nmethods phases.
-      wait_until_all_strong_classes_discovered();
-    }
-
-    // Now take the complement of the strong CLDs.
-    G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::WeakCLDRoots, worker_i);
-    assert(closures->second_pass_weak_clds() != NULL, "Should be non-null if we are tracing metadata.");
-    ClassLoaderDataGraph::roots_cld_do(NULL, closures->second_pass_weak_clds());
-  } else {
-    phase_times->record_time_secs(G1GCPhaseTimes::WaitForStrongCLD, worker_i, 0.0);
-    phase_times->record_time_secs(G1GCPhaseTimes::WeakCLDRoots, worker_i, 0.0);
-    assert(closures->second_pass_weak_clds() == NULL, "Should be null if not tracing metadata.");
   }
 
   _process_strong_tasks.all_tasks_completed(n_workers());
@@ -189,17 +171,24 @@ void G1RootProcessor::process_all_roots(OopClosure* oops,
 
 void G1RootProcessor::process_java_roots(G1RootClosures* closures,
                                          G1GCPhaseTimes* phase_times,
-                                         uint worker_i) {
+                                         uint worker_i,
-  // Iterating over the CLDG and the Threads are done early to allow us to
+                                         bool notify_claimed_nmethods_done) {
-  // first process the strong CLDs and nmethods and then, after a barrier,
+  // We need to make make sure that the "strong" nmethods are processed first
-  // let the thread process the weak CLDs and nmethods.
+  // using the strong closure. Only after that we process the weakly reachable
-  {
+  // nmethods.
-    G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::CLDGRoots, worker_i);
+  // We need to strictly separate the strong and weak nmethod processing because
-    if (_process_strong_tasks.try_claim_task(G1RP_PS_ClassLoaderDataGraph_oops_do)) {
+  // any processing claims that nmethod, i.e. will not be iterated again.
-      ClassLoaderDataGraph::roots_cld_do(closures->strong_clds(), closures->weak_clds());
+  // Which means if an nmethod is processed first and claimed, the strong processing
-    }
+  // will not happen, and the oops reachable by that nmethod will not be marked
-  }
+  // properly.
-
+  //
+  // That is why we process strong nmethods first, synchronize all threads via a
+  // barrier, and only then allow weak processing. To minimize the wait time at
+  // that barrier we do the strong nmethod processing first, and immediately after-
+  // wards indicate that that thread is done. Hopefully other root processing after
+  // nmethod processing is enough so there is no need to wait.
+  //
+  // This is only required in the concurrent start pause with class unloading enabled.
   {
     G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::ThreadRoots, worker_i);
     bool is_par = n_workers() > 1;
@@ -207,6 +196,19 @@ void G1RootProcessor::process_java_roots(G1RootClosures* closures,
                                        closures->strong_oops(),
                                        closures->strong_codeblobs());
   }
+
+  // This is the point where this worker thread will not find more strong nmethods.
+  // Report this so G1 can synchronize the strong and weak CLDs/nmethods processing.
+  if (notify_claimed_nmethods_done) {
+    worker_has_discovered_all_strong_nmethods();
+  }
+
+  {
+    G1GCParPhaseTimesTracker x(phase_times, G1GCPhaseTimes::CLDGRoots, worker_i);
+    if (_process_strong_tasks.try_claim_task(G1RP_PS_ClassLoaderDataGraph_oops_do)) {
+      ClassLoaderDataGraph::roots_cld_do(closures->strong_clds(), closures->weak_clds());
+    }
+  }
 }
 
 void G1RootProcessor::process_vm_roots(G1RootClosures* closures,

src/hotspot/share/gc/g1/g1RootProcessor.hpp

@@ -69,12 +69,13 @@ class G1RootProcessor : public StackObj {
     G1RP_PS_NumElements
   };
 
-  void worker_has_discovered_all_strong_classes();
+  void worker_has_discovered_all_strong_nmethods();
-  void wait_until_all_strong_classes_discovered();
+  void wait_until_all_strong_nmethods_discovered();
 
   void process_java_roots(G1RootClosures* closures,
                           G1GCPhaseTimes* phase_times,
-                          uint worker_i);
+                          uint worker_i,
+                          bool notify_claimed_nmethods_done = false);
 
   void process_vm_roots(G1RootClosures* closures,
                         G1GCPhaseTimes* phase_times,

src/hotspot/share/gc/g1/g1SharedClosures.hpp

@@ -39,9 +39,9 @@ public:
   G1CLDScanClosure                _clds;
   G1CodeBlobClosure               _codeblobs;
 
-  G1SharedClosures(G1CollectedHeap* g1h, G1ParScanThreadState* pss, bool process_only_dirty, int cld_claim) :
+  G1SharedClosures(G1CollectedHeap* g1h, G1ParScanThreadState* pss, bool process_only_dirty) :
     _oops(g1h, pss),
     _oops_in_cld(g1h, pss),
-    _clds(&_oops_in_cld, process_only_dirty, cld_claim),
+    _clds(&_oops_in_cld, process_only_dirty),
     _codeblobs(&_oops) {}
 };

test/hotspot/jtreg/gc/g1/TestGCLogMessages.java

@@ -128,8 +128,7 @@ public class TestGCLogMessages {
         new LogMessageWithLevel("CLDG Roots", Level.TRACE),
         new LogMessageWithLevel("JVMTI Roots", Level.TRACE),
         new LogMessageWithLevel("CM RefProcessor Roots", Level.TRACE),
-        new LogMessageWithLevel("Wait For Strong CLD", Level.TRACE),
+        new LogMessageWithLevel("Wait For Strong Roots", Level.TRACE),
-        new LogMessageWithLevel("Weak CLD Roots", Level.TRACE),
         // Redirty Cards
         new LogMessageWithLevel("Redirty Cards", Level.DEBUG),
         new LogMessageWithLevel("Parallel Redirty", Level.TRACE),

test/jdk/jdk/jfr/event/gc/collection/TestG1ParallelPhases.java

@@ -98,8 +98,7 @@ public class TestG1ParallelPhases {
             "CLDGRoots",
             "JVMTIRoots",
             "CMRefRoots",
-            "WaitForStrongCLD",
+            "WaitForStrongRoots",
-            "WeakCLDRoots",
             "MergeER",
             "MergeHCC",
             "MergeRS",