8189748: More precise closures for WeakProcessor::weak_oops_do calls

Reviewed-by: pliden, sjohanss

src/hotspot/share/compiler/oopMap.cpp

@@ -30,6 +30,7 @@
 #include "compiler/oopMap.hpp"
 #include "gc/shared/collectedHeap.hpp"
 #include "memory/allocation.inline.hpp"
+#include "memory/iterator.hpp"
 #include "memory/resourceArea.hpp"
 #include "runtime/frame.inline.hpp"
 #include "runtime/signature.hpp"
@@ -263,13 +264,6 @@ OopMap* OopMapSet::find_map_at_offset(int pc_offset) const {
   return m;
 }
 
-class DoNothingClosure: public OopClosure {
- public:
-  void do_oop(oop* p)       {}
-  void do_oop(narrowOop* p) {}
-};
-static DoNothingClosure do_nothing;
-
 static void add_derived_oop(oop* base, oop* derived) {
 #if !defined(TIERED) && !defined(INCLUDE_JVMCI)
   COMPILER1_PRESENT(ShouldNotReachHere();)
@@ -310,7 +304,7 @@ static void trace_codeblob_maps(const frame *fr, const RegisterMap *reg_map) {
 
 void OopMapSet::oops_do(const frame *fr, const RegisterMap* reg_map, OopClosure* f) {
   // add derived oops to a table
-  all_do(fr, reg_map, f, add_derived_oop, &do_nothing);
+  all_do(fr, reg_map, f, add_derived_oop, &do_nothing_cl);
 }
 
 

src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp

@@ -5181,14 +5181,16 @@ void CMSCollector::refProcessingWork() {
   rp->setup_policy(false);
   verify_work_stacks_empty();
 
+  ReferenceProcessorPhaseTimes pt(_gc_timer_cm, rp->num_q());
+  {
+    GCTraceTime(Debug, gc, phases) t("Reference Processing", _gc_timer_cm);
+
+    // Setup keep_alive and complete closures.
     CMSKeepAliveClosure cmsKeepAliveClosure(this, _span, &_markBitMap,
                                             &_markStack, false /* !preclean */);
     CMSDrainMarkingStackClosure cmsDrainMarkingStackClosure(this,
                                   _span, &_markBitMap, &_markStack,
                                   &cmsKeepAliveClosure, false /* !preclean */);
-  ReferenceProcessorPhaseTimes pt(_gc_timer_cm, rp->num_q());
-  {
-    GCTraceTime(Debug, gc, phases) t("Reference Processing", _gc_timer_cm);
 
     ReferenceProcessorStats stats;
     if (rp->processing_is_mt()) {
@@ -5225,14 +5227,14 @@ void CMSCollector::refProcessingWork() {
     pt.print_all_references();
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) t("Weak Processing", _gc_timer_cm);
-    WeakProcessor::weak_oops_do(&_is_alive_closure, &cmsKeepAliveClosure, &cmsDrainMarkingStackClosure);
-  }
-
   // This is the point where the entire marking should have completed.
   verify_work_stacks_empty();
 
+  {
+    GCTraceTime(Debug, gc, phases) t("Weak Processing", _gc_timer_cm);
+    WeakProcessor::weak_oops_do(&_is_alive_closure, &do_nothing_cl);
+  }
+
   if (should_unload_classes()) {
     {
       GCTraceTime(Debug, gc, phases) t("Class Unloading", _gc_timer_cm);

src/hotspot/share/gc/cms/parNewGeneration.cpp

@@ -1000,7 +1000,13 @@ void ParNewGeneration::collect(bool   full,
   _gc_tracer.report_tenuring_threshold(tenuring_threshold());
   pt.print_all_references();
 
-  WeakProcessor::weak_oops_do(&is_alive, &keep_alive, &evacuate_followers);
+  assert(gch->no_allocs_since_save_marks(), "evacuation should be done at this point");
+
+  WeakProcessor::weak_oops_do(&is_alive, &keep_alive);
+
+  // Verify that the usage of keep_alive only forwarded
+  // the oops and did not find anything new to copy.
+  assert(gch->no_allocs_since_save_marks(), "unexpectedly copied objects");
 
   if (!promotion_failed()) {
     // Swap the survivor spaces.

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

@@ -1604,6 +1604,20 @@ void G1ConcurrentMark::weakRefsWork(bool clear_all_soft_refs) {
   // Is alive closure.
   G1CMIsAliveClosure g1_is_alive(g1h);
 
+  // Inner scope to exclude the cleaning of the string and symbol
+  // tables from the displayed time.
+  {
+    GCTraceTime(Debug, gc, phases) trace("Reference Processing", _gc_timer_cm);
+
+    ReferenceProcessor* rp = g1h->ref_processor_cm();
+
+    // See the comment in G1CollectedHeap::ref_processing_init()
+    // about how reference processing currently works in G1.
+
+    // Set the soft reference policy
+    rp->setup_policy(clear_all_soft_refs);
+    assert(_global_mark_stack.is_empty(), "mark stack should be empty");
+
     // Instances of the 'Keep Alive' and 'Complete GC' closures used
     // in serial reference processing. Note these closures are also
     // used for serially processing (by the the current thread) the
@@ -1621,20 +1635,6 @@ void G1ConcurrentMark::weakRefsWork(bool clear_all_soft_refs) {
     G1CMKeepAliveAndDrainClosure g1_keep_alive(this, task(0), true /* is_serial */);
     G1CMDrainMarkingStackClosure g1_drain_mark_stack(this, task(0), true /* is_serial */);
 
-  // Inner scope to exclude the cleaning of the string and symbol
-  // tables from the displayed time.
-  {
-    GCTraceTime(Debug, gc, phases) trace("Reference Processing", _gc_timer_cm);
-
-    ReferenceProcessor* rp = g1h->ref_processor_cm();
-
-    // See the comment in G1CollectedHeap::ref_processing_init()
-    // about how reference processing currently works in G1.
-
-    // Set the soft reference policy
-    rp->setup_policy(clear_all_soft_refs);
-    assert(_global_mark_stack.is_empty(), "mark stack should be empty");
-
     // We need at least one active thread. If reference processing
     // is not multi-threaded we use the current (VMThread) thread,
     // otherwise we use the work gang from the G1CollectedHeap and
@@ -1688,9 +1688,12 @@ void G1ConcurrentMark::weakRefsWork(bool clear_all_soft_refs) {
     assert(!rp->discovery_enabled(), "Post condition");
   }
 
+  assert(has_overflown() || _global_mark_stack.is_empty(),
+          "Mark stack should be empty (unless it has overflown)");
+
   {
     GCTraceTime(Debug, gc, phases) debug("Weak Processing", _gc_timer_cm);
-    WeakProcessor::weak_oops_do(&g1_is_alive, &g1_keep_alive, &g1_drain_mark_stack);
+    WeakProcessor::weak_oops_do(&g1_is_alive, &do_nothing_cl);
   }
 
   if (has_overflown()) {

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

@@ -182,16 +182,14 @@ void G1MarkSweep::mark_sweep_phase1(bool& marked_for_unloading,
     pt.print_all_references();
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) trace("Weak Processing", gc_timer());
-    WeakProcessor::weak_oops_do(&GenMarkSweep::is_alive,
-                                &GenMarkSweep::keep_alive,
-                                &GenMarkSweep::follow_stack_closure);
-  }
-
   // This is the point where the entire marking should have completed.
   assert(GenMarkSweep::_marking_stack.is_empty(), "Marking should have completed");
 
+  {
+    GCTraceTime(Debug, gc, phases) trace("Weak Processing", gc_timer());
+    WeakProcessor::weak_oops_do(&GenMarkSweep::is_alive, &do_nothing_cl);
+  }
+
   if (ClassUnloading) {
     GCTraceTime(Debug, gc, phases) trace("Class Unloading", gc_timer());
 

src/hotspot/share/gc/parallel/psMarkSweep.cpp

@@ -543,14 +543,14 @@ void PSMarkSweep::mark_sweep_phase1(bool clear_all_softrefs) {
     pt.print_all_references();
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) t("Weak Processing", _gc_timer);
-    WeakProcessor::weak_oops_do(is_alive_closure(), mark_and_push_closure(), follow_stack_closure());
-  }
-
   // This is the point where the entire marking should have completed.
   assert(_marking_stack.is_empty(), "Marking should have completed");
 
+  {
+    GCTraceTime(Debug, gc, phases) t("Weak Processing", _gc_timer);
+    WeakProcessor::weak_oops_do(is_alive_closure(), &do_nothing_cl);
+  }
+
   {
     GCTraceTime(Debug, gc, phases) t("Class Unloading", _gc_timer);
 

src/hotspot/share/gc/parallel/psParallelCompact.cpp

@@ -2119,14 +2119,14 @@ void PSParallelCompact::marking_phase(ParCompactionManager* cm,
     pt.print_all_references();
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) tm("Weak Processing", &_gc_timer);
-    WeakProcessor::weak_oops_do(is_alive_closure(), &mark_and_push_closure, &follow_stack_closure);
-  }
-
   // This is the point where the entire marking should have completed.
   assert(cm->marking_stacks_empty(), "Marking should have completed");
 
+  {
+    GCTraceTime(Debug, gc, phases) tm("Weak Processing", &_gc_timer);
+    WeakProcessor::weak_oops_do(is_alive_closure(), &do_nothing_cl);
+  }
+
   {
     GCTraceTime(Debug, gc, phases) tm_m("Class Unloading", &_gc_timer);
 

src/hotspot/share/gc/parallel/psScavenge.cpp

@@ -407,15 +407,14 @@ bool PSScavenge::invoke_no_policy() {
 
     scavenge_midpoint.update();
 
-    PSKeepAliveClosure keep_alive(promotion_manager);
-    PSEvacuateFollowersClosure evac_followers(promotion_manager);
-
     // Process reference objects discovered during scavenge
     {
       GCTraceTime(Debug, gc, phases) tm("Reference Processing", &_gc_timer);
 
       reference_processor()->setup_policy(false); // not always_clear
       reference_processor()->set_active_mt_degree(active_workers);
+      PSKeepAliveClosure keep_alive(promotion_manager);
+      PSEvacuateFollowersClosure evac_followers(promotion_manager);
       ReferenceProcessorStats stats;
       ReferenceProcessorPhaseTimes pt(&_gc_timer, reference_processor()->num_q());
       if (reference_processor()->processing_is_mt()) {
@@ -442,18 +441,24 @@ bool PSScavenge::invoke_no_policy() {
       pt.print_enqueue_phase();
     }
 
+    assert(promotion_manager->stacks_empty(),"stacks should be empty at this point");
+
+    PSScavengeRootsClosure root_closure(promotion_manager);
+
     {
       GCTraceTime(Debug, gc, phases) tm("Weak Processing", &_gc_timer);
-      WeakProcessor::weak_oops_do(&_is_alive_closure, &keep_alive, &evac_followers);
+      WeakProcessor::weak_oops_do(&_is_alive_closure, &root_closure);
     }
 
     {
       GCTraceTime(Debug, gc, phases) tm("Scrub String Table", &_gc_timer);
       // Unlink any dead interned Strings and process the remaining live ones.
-      PSScavengeRootsClosure root_closure(promotion_manager);
       StringTable::unlink_or_oops_do(&_is_alive_closure, &root_closure);
     }
 
+    // Verify that usage of root_closure didn't copy any objects.
+    assert(promotion_manager->stacks_empty(),"stacks should be empty at this point");
+
     // Finally, flush the promotion_manager's labs, and deallocate its stacks.
     promotion_failure_occurred = PSPromotionManager::post_scavenge(_gc_tracer);
     if (promotion_failure_occurred) {

src/hotspot/share/gc/serial/defNewGeneration.cpp

@@ -659,7 +659,12 @@ void DefNewGeneration::collect(bool   full,
   gc_tracer.report_tenuring_threshold(tenuring_threshold());
   pt.print_all_references();
 
-  WeakProcessor::weak_oops_do(&is_alive, &keep_alive, &evacuate_followers);
+  assert(gch->no_allocs_since_save_marks(), "save marks have not been newly set.");
+
+  WeakProcessor::weak_oops_do(&is_alive, &keep_alive);
+
+  // Verify that the usage of keep_alive didn't copy any objects.
+  assert(gch->no_allocs_since_save_marks(), "save marks have not been newly set.");
 
   if (!_promotion_failed) {
     // Swap the survivor spaces.

src/hotspot/share/gc/serial/genMarkSweep.cpp

@@ -218,14 +218,14 @@ void GenMarkSweep::mark_sweep_phase1(bool clear_all_softrefs) {
     gc_tracer()->report_gc_reference_stats(stats);
   }
 
-  {
-    GCTraceTime(Debug, gc, phases) tm_m("Weak Processing", gc_timer());
-    WeakProcessor::weak_oops_do(&is_alive, &keep_alive, &follow_stack_closure);
-  }
-
   // This is the point where the entire marking should have completed.
   assert(_marking_stack.is_empty(), "Marking should have completed");
 
+  {
+    GCTraceTime(Debug, gc, phases) tm_m("Weak Processing", gc_timer());
+    WeakProcessor::weak_oops_do(&is_alive, &do_nothing_cl);
+  }
+
   {
     GCTraceTime(Debug, gc, phases) tm_m("Class Unloading", gc_timer());
 

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

@@ -27,16 +27,12 @@
 #include "prims/jvmtiExport.hpp"
 #include "runtime/jniHandles.hpp"
 
-void WeakProcessor::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, VoidClosure* complete) {
+void WeakProcessor::weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive) {
   JNIHandles::weak_oops_do(is_alive, keep_alive);
   JvmtiExport::weak_oops_do(is_alive, keep_alive);
-
-  if (complete != NULL) {
-    complete->do_void();
-  }
 }
 
 void WeakProcessor::oops_do(OopClosure* closure) {
   AlwaysTrueClosure always_true;
-  weak_oops_do(&always_true, closure, NULL);
+  weak_oops_do(&always_true, closure);
 }

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

@@ -37,10 +37,7 @@ public:
   // Visit all oop*s and apply the keep_alive closure if the referenced
   // object is considered alive by the is_alive closure, otherwise do some
   // container specific cleanup of element holding the oop.
-  //
+  static void weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive);
-  // The complete closure is used as a post-processing step,
-  // called after all container have been processed.
-  static void weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive, VoidClosure* complete = NULL);
 
   // Visit all oop*s and apply the given closure.
   static void oops_do(OopClosure* closure);

src/hotspot/share/memory/iterator.cpp

@@ -29,6 +29,8 @@
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 
+DoNothingClosure do_nothing_cl;
+
 void CLDToOopClosure::do_cld(ClassLoaderData* cld) {
   cld->oops_do(_oop_closure, _must_claim_cld);
 }

src/hotspot/share/memory/iterator.hpp

@@ -48,6 +48,13 @@ class OopClosure : public Closure {
   virtual void do_oop(narrowOop* o) = 0;
 };
 
+class DoNothingClosure : public OopClosure {
+ public:
+  virtual void do_oop(oop* p)       {}
+  virtual void do_oop(narrowOop* p) {}
+};
+extern DoNothingClosure do_nothing_cl;
+
 // ExtendedOopClosure adds extra code to be run during oop iterations.
 // This is needed by the GC and is extracted to a separate type to not
 // pollute the OopClosure interface.