8189748: More precise closures for WeakProcessor::weak_oops_do calls
authorstefank
Mon, 23 Oct 2017 11:20:53 +0200
changeset 47676 b1c020fc35a3
parent 47674 1587ffa1496a
child 47677 7300cb446de8
8189748: More precise closures for WeakProcessor::weak_oops_do calls Reviewed-by: pliden, sjohanss
src/hotspot/share/compiler/oopMap.cpp
src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
src/hotspot/share/gc/cms/parNewGeneration.cpp
src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
src/hotspot/share/gc/g1/g1MarkSweep.cpp
src/hotspot/share/gc/parallel/psMarkSweep.cpp
src/hotspot/share/gc/parallel/psParallelCompact.cpp
src/hotspot/share/gc/parallel/psScavenge.cpp
src/hotspot/share/gc/serial/defNewGeneration.cpp
src/hotspot/share/gc/serial/genMarkSweep.cpp
src/hotspot/share/gc/shared/weakProcessor.cpp
src/hotspot/share/gc/shared/weakProcessor.hpp
src/hotspot/share/memory/iterator.cpp
src/hotspot/share/memory/iterator.hpp
--- a/src/hotspot/share/compiler/oopMap.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/compiler/oopMap.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -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 @@
   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 @@
 
 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);
 }
 
 
--- a/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -5181,15 +5181,17 @@
   rp->setup_policy(false);
   verify_work_stacks_empty();
 
-  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);
 
+    // Setup keep_alive and complete closures.
+    CMSKeepAliveClosure cmsKeepAliveClosure(this, _span, &_markBitMap,
+                                            &_markStack, false /* !preclean */);
+    CMSDrainMarkingStackClosure cmsDrainMarkingStackClosure(this,
+                                  _span, &_markBitMap, &_markStack,
+                                  &cmsKeepAliveClosure, false /* !preclean */);
+
     ReferenceProcessorStats stats;
     if (rp->processing_is_mt()) {
       // Set the degree of MT here.  If the discovery is done MT, there
@@ -5225,13 +5227,13 @@
     pt.print_all_references();
   }
 
+  // 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, &cmsKeepAliveClosure, &cmsDrainMarkingStackClosure);
-  }
-
-  // This is the point where the entire marking should have completed.
-  verify_work_stacks_empty();
+    WeakProcessor::weak_oops_do(&_is_alive_closure, &do_nothing_cl);
+  }
 
   if (should_unload_classes()) {
     {
--- a/src/hotspot/share/gc/cms/parNewGeneration.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/cms/parNewGeneration.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -1000,7 +1000,13 @@
   _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.
--- a/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/g1/g1ConcurrentMark.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -1604,23 +1604,6 @@
   // Is alive closure.
   G1CMIsAliveClosure g1_is_alive(g1h);
 
-  // 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
-  // JNI references during parallel reference processing.
-  //
-  // These closures do not need to synchronize with the worker
-  // threads involved in parallel reference processing as these
-  // instances are executed serially by the current thread (e.g.
-  // reference processing is not multi-threaded and is thus
-  // performed by the current thread instead of a gang worker).
-  //
-  // The gang tasks involved in parallel reference processing create
-  // their own instances of these closures, which do their own
-  // synchronization among themselves.
-  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.
   {
@@ -1635,6 +1618,23 @@
     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
+    // JNI references during parallel reference processing.
+    //
+    // These closures do not need to synchronize with the worker
+    // threads involved in parallel reference processing as these
+    // instances are executed serially by the current thread (e.g.
+    // reference processing is not multi-threaded and is thus
+    // performed by the current thread instead of a gang worker).
+    //
+    // The gang tasks involved in parallel reference processing create
+    // their own instances of these closures, which do their own
+    // synchronization among themselves.
+    G1CMKeepAliveAndDrainClosure g1_keep_alive(this, task(0), true /* is_serial */);
+    G1CMDrainMarkingStackClosure g1_drain_mark_stack(this, task(0), true /* is_serial */);
+
     // 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 @@
     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()) {
--- a/src/hotspot/share/gc/g1/g1MarkSweep.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/g1/g1MarkSweep.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -182,16 +182,14 @@
     pt.print_all_references();
   }
 
+  // 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,
-                                &GenMarkSweep::keep_alive,
-                                &GenMarkSweep::follow_stack_closure);
+    WeakProcessor::weak_oops_do(&GenMarkSweep::is_alive, &do_nothing_cl);
   }
 
-  // This is the point where the entire marking should have completed.
-  assert(GenMarkSweep::_marking_stack.is_empty(), "Marking should have completed");
-
   if (ClassUnloading) {
     GCTraceTime(Debug, gc, phases) trace("Class Unloading", gc_timer());
 
--- a/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/parallel/psMarkSweep.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -543,14 +543,14 @@
     pt.print_all_references();
   }
 
+  // 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(), mark_and_push_closure(), follow_stack_closure());
+    WeakProcessor::weak_oops_do(is_alive_closure(), &do_nothing_cl);
   }
 
-  // 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("Class Unloading", _gc_timer);
 
--- a/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/parallel/psParallelCompact.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -2119,14 +2119,14 @@
     pt.print_all_references();
   }
 
+  // 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(), &mark_and_push_closure, &follow_stack_closure);
+    WeakProcessor::weak_oops_do(is_alive_closure(), &do_nothing_cl);
   }
 
-  // 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_m("Class Unloading", &_gc_timer);
 
--- a/src/hotspot/share/gc/parallel/psScavenge.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/parallel/psScavenge.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -407,15 +407,14 @@
 
     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 @@
       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) {
--- a/src/hotspot/share/gc/serial/defNewGeneration.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/serial/defNewGeneration.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -659,7 +659,12 @@
   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.
--- a/src/hotspot/share/gc/serial/genMarkSweep.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/serial/genMarkSweep.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -218,14 +218,14 @@
     gc_tracer()->report_gc_reference_stats(stats);
   }
 
+  // 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, &keep_alive, &follow_stack_closure);
+    WeakProcessor::weak_oops_do(&is_alive, &do_nothing_cl);
   }
 
-  // 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("Class Unloading", gc_timer());
 
--- a/src/hotspot/share/gc/shared/weakProcessor.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/shared/weakProcessor.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -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);
 }
--- a/src/hotspot/share/gc/shared/weakProcessor.hpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/gc/shared/weakProcessor.hpp	Mon Oct 23 11:20:53 2017 +0200
@@ -37,10 +37,7 @@
   // 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.
-  //
-  // 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);
+  static void weak_oops_do(BoolObjectClosure* is_alive, OopClosure* keep_alive);
 
   // Visit all oop*s and apply the given closure.
   static void oops_do(OopClosure* closure);
--- a/src/hotspot/share/memory/iterator.cpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/memory/iterator.cpp	Mon Oct 23 11:20:53 2017 +0200
@@ -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);
 }
--- a/src/hotspot/share/memory/iterator.hpp	Mon Oct 23 03:15:19 2017 -0400
+++ b/src/hotspot/share/memory/iterator.hpp	Mon Oct 23 11:20:53 2017 +0200
@@ -48,6 +48,13 @@
   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.