changeset 46307:686d50172bfd

8071278: Fix the closure mess in G1RemSet::refine_card() Summary: Remove the use of many nested closure in the code to refine a card. Reviewed-by: kbarrett, sjohanss
author tschatzl
date Mon, 06 Mar 2017 17:03:35 +0100
parents 26cf11456712
children 8b88f3db98b8
files hotspot/src/share/vm/gc/g1/g1OopClosures.hpp hotspot/src/share/vm/gc/g1/g1OopClosures.inline.hpp hotspot/src/share/vm/gc/g1/g1RemSet.cpp hotspot/src/share/vm/gc/g1/g1_specialized_oop_closures.hpp hotspot/src/share/vm/gc/g1/heapRegion.cpp hotspot/src/share/vm/gc/g1/heapRegion.hpp
diffstat 6 files changed, 49 insertions(+), 169 deletions(-) [+]
line wrap: on
line diff
--- a/hotspot/src/share/vm/gc/g1/g1OopClosures.hpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/g1OopClosures.hpp	Mon Mar 06 17:03:35 2017 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -143,29 +143,6 @@
   void do_klass(Klass* klass);
 };
 
-class FilterIntoCSClosure: public OopClosure {
-  G1CollectedHeap* _g1;
-  OopClosure* _oc;
-public:
-  FilterIntoCSClosure(G1CollectedHeap* g1, OopClosure* oc) : _g1(g1), _oc(oc) { }
-
-  template <class T> void do_oop_work(T* p);
-  virtual void do_oop(oop* p)        { do_oop_work(p); }
-  virtual void do_oop(narrowOop* p)  { do_oop_work(p); }
-};
-
-class FilterOutOfRegionClosure: public ExtendedOopClosure {
-  HeapWord* _r_bottom;
-  HeapWord* _r_end;
-  OopClosure* _oc;
-public:
-  FilterOutOfRegionClosure(HeapRegion* r, OopClosure* oc);
-  template <class T> void do_oop_nv(T* p);
-  virtual void do_oop(oop* p) { do_oop_nv(p); }
-  virtual void do_oop(narrowOop* p) { do_oop_nv(p); }
-  bool apply_to_weak_ref_discovered_field() { return true; }
-};
-
 // Closure for iterating over object fields during concurrent marking
 class G1CMOopClosure : public MetadataAwareOopClosure {
 protected:
@@ -193,58 +170,16 @@
   virtual void do_oop(narrowOop* p) { do_oop_nv(p); }
 };
 
-// Closure that applies the given two closures in sequence.
-// Used by the RSet refinement code (when updating RSets
-// during an evacuation pause) to record cards containing
-// pointers into the collection set.
-
-class G1Mux2Closure : public OopClosure {
-  OopClosure* _c1;
-  OopClosure* _c2;
-public:
-  G1Mux2Closure(OopClosure *c1, OopClosure *c2);
-  template <class T> inline void do_oop_work(T* p);
-  virtual inline void do_oop(oop* p);
-  virtual inline void do_oop(narrowOop* p);
-};
-
-// A closure that returns true if it is actually applied
-// to a reference
-
-class G1TriggerClosure : public OopClosure {
-  bool _triggered;
-public:
-  G1TriggerClosure();
-  bool triggered() const { return _triggered; }
-  template <class T> inline void do_oop_work(T* p);
-  virtual inline void do_oop(oop* p);
-  virtual inline void do_oop(narrowOop* p);
-};
-
-// A closure which uses a triggering closure to determine
-// whether to apply an oop closure.
-
-class G1InvokeIfNotTriggeredClosure: public OopClosure {
-  G1TriggerClosure* _trigger_cl;
-  OopClosure* _oop_cl;
-public:
-  G1InvokeIfNotTriggeredClosure(G1TriggerClosure* t, OopClosure* oc);
-  template <class T> inline void do_oop_work(T* p);
-  virtual inline void do_oop(oop* p);
-  virtual inline void do_oop(narrowOop* p);
-};
-
-class G1UpdateRSOrPushRefOopClosure: public OopClosure {
+class G1UpdateRSOrPushRefOopClosure: public ExtendedOopClosure {
   G1CollectedHeap* _g1;
-  G1RemSet* _g1_rem_set;
   HeapRegion* _from;
   G1ParPushHeapRSClosure* _push_ref_cl;
   bool _record_refs_into_cset;
   uint _worker_i;
+  bool _has_refs_into_cset;
 
 public:
   G1UpdateRSOrPushRefOopClosure(G1CollectedHeap* g1h,
-                                G1RemSet* rs,
                                 G1ParPushHeapRSClosure* push_ref_cl,
                                 bool record_refs_into_cset,
                                 uint worker_i = 0);
@@ -254,13 +189,17 @@
     _from = from;
   }
 
+  bool apply_to_weak_ref_discovered_field() { return true; }
+
   bool self_forwarded(oop obj) {
     markOop m = obj->mark();
     bool result = (m->is_marked() && ((oop)m->decode_pointer() == obj));
     return result;
   }
 
-  template <class T> inline void do_oop_work(T* p);
+  bool has_refs_into_cset() const { return _has_refs_into_cset; }
+
+  template <class T> inline void do_oop_nv(T* p);
   virtual inline void do_oop(narrowOop* p);
   virtual inline void do_oop(oop* p);
 };
--- a/hotspot/src/share/vm/gc/g1/g1OopClosures.inline.hpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/g1OopClosures.inline.hpp	Mon Mar 06 17:03:35 2017 +0100
@@ -36,31 +36,6 @@
 #include "memory/iterator.inline.hpp"
 #include "runtime/prefetch.inline.hpp"
 
-/*
- * This really ought to be an inline function, but apparently the C++
- * compiler sometimes sees fit to ignore inline declarations.  Sigh.
- */
-
-template <class T>
-inline void FilterIntoCSClosure::do_oop_work(T* p) {
-  T heap_oop = oopDesc::load_heap_oop(p);
-  if (!oopDesc::is_null(heap_oop) &&
-      _g1->is_in_cset_or_humongous(oopDesc::decode_heap_oop_not_null(heap_oop))) {
-    _oc->do_oop(p);
-  }
-}
-
-template <class T>
-inline void FilterOutOfRegionClosure::do_oop_nv(T* p) {
-  T heap_oop = oopDesc::load_heap_oop(p);
-  if (!oopDesc::is_null(heap_oop)) {
-    HeapWord* obj_hw = (HeapWord*)oopDesc::decode_heap_oop_not_null(heap_oop);
-    if (obj_hw < _r_bottom || obj_hw >= _r_end) {
-      _oc->do_oop(p);
-    }
-  }
-}
-
 // This closure is applied to the fields of the objects that have just been copied.
 template <class T>
 inline void G1ParScanClosure::do_oop_nv(T* p) {
@@ -136,33 +111,7 @@
 }
 
 template <class T>
-inline void G1Mux2Closure::do_oop_work(T* p) {
-  // Apply first closure; then apply the second.
-  _c1->do_oop(p);
-  _c2->do_oop(p);
-}
-void G1Mux2Closure::do_oop(oop* p)       { do_oop_work(p); }
-void G1Mux2Closure::do_oop(narrowOop* p) { do_oop_work(p); }
-
-template <class T>
-inline void G1TriggerClosure::do_oop_work(T* p) {
-  // Record that this closure was actually applied (triggered).
-  _triggered = true;
-}
-void G1TriggerClosure::do_oop(oop* p)       { do_oop_work(p); }
-void G1TriggerClosure::do_oop(narrowOop* p) { do_oop_work(p); }
-
-template <class T>
-inline void G1InvokeIfNotTriggeredClosure::do_oop_work(T* p) {
-  if (!_trigger_cl->triggered()) {
-    _oop_cl->do_oop(p);
-  }
-}
-void G1InvokeIfNotTriggeredClosure::do_oop(oop* p)       { do_oop_work(p); }
-void G1InvokeIfNotTriggeredClosure::do_oop(narrowOop* p) { do_oop_work(p); }
-
-template <class T>
-inline void G1UpdateRSOrPushRefOopClosure::do_oop_work(T* p) {
+inline void G1UpdateRSOrPushRefOopClosure::do_oop_nv(T* p) {
   oop obj = oopDesc::load_decode_heap_oop(p);
   if (obj == NULL) {
     return;
@@ -217,6 +166,7 @@
       // instance for this worker thread.
       _push_ref_cl->do_oop(p);
     }
+    _has_refs_into_cset = true;
 
     // Deferred updates to the CSet are either discarded (in the normal case),
     // or processed (if an evacuation failure occurs) at the end
@@ -232,8 +182,8 @@
     to->rem_set()->add_reference(p, _worker_i);
   }
 }
-void G1UpdateRSOrPushRefOopClosure::do_oop(oop* p)       { do_oop_work(p); }
-void G1UpdateRSOrPushRefOopClosure::do_oop(narrowOop* p) { do_oop_work(p); }
+void G1UpdateRSOrPushRefOopClosure::do_oop(oop* p)       { do_oop_nv(p); }
+void G1UpdateRSOrPushRefOopClosure::do_oop(narrowOop* p) { do_oop_nv(p); }
 
 template <class T>
 void G1ParCopyHelper::do_klass_barrier(T* p, oop new_obj) {
--- a/hotspot/src/share/vm/gc/g1/g1RemSet.cpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/g1RemSet.cpp	Mon Mar 06 17:03:35 2017 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -527,25 +527,16 @@
   _g1->heap_region_par_iterate(&scrub_cl, worker_num, hrclaimer);
 }
 
-G1TriggerClosure::G1TriggerClosure() :
-  _triggered(false) { }
-
-G1InvokeIfNotTriggeredClosure::G1InvokeIfNotTriggeredClosure(G1TriggerClosure* t_cl,
-                                                             OopClosure* oop_cl)  :
-  _trigger_cl(t_cl), _oop_cl(oop_cl) { }
-
-G1Mux2Closure::G1Mux2Closure(OopClosure *c1, OopClosure *c2) :
-  _c1(c1), _c2(c2) { }
-
-G1UpdateRSOrPushRefOopClosure::
-G1UpdateRSOrPushRefOopClosure(G1CollectedHeap* g1h,
-                              G1RemSet* rs,
-                              G1ParPushHeapRSClosure* push_ref_cl,
-                              bool record_refs_into_cset,
-                              uint worker_i) :
-  _g1(g1h), _g1_rem_set(rs), _from(NULL),
+G1UpdateRSOrPushRefOopClosure::G1UpdateRSOrPushRefOopClosure(G1CollectedHeap* g1h,
+                                                             G1ParPushHeapRSClosure* push_ref_cl,
+                                                             bool record_refs_into_cset,
+                                                             uint worker_i) :
+  _g1(g1h),
+  _from(NULL),
   _record_refs_into_cset(record_refs_into_cset),
-  _push_ref_cl(push_ref_cl), _worker_i(worker_i) { }
+  _has_refs_into_cset(false),
+  _push_ref_cl(push_ref_cl),
+  _worker_i(worker_i) { }
 
 // Returns true if the given card contains references that point
 // into the collection set, if we're checking for such references;
@@ -690,25 +681,14 @@
   assert(!dirty_region.is_empty(), "sanity");
 
   G1UpdateRSOrPushRefOopClosure update_rs_oop_cl(_g1,
-                                                 _g1->g1_rem_set(),
                                                  oops_in_heap_closure,
                                                  check_for_refs_into_cset,
                                                  worker_i);
   update_rs_oop_cl.set_from(r);
 
-  G1TriggerClosure trigger_cl;
-  FilterIntoCSClosure into_cs_cl(_g1, &trigger_cl);
-  G1InvokeIfNotTriggeredClosure invoke_cl(&trigger_cl, &into_cs_cl);
-  G1Mux2Closure mux(&invoke_cl, &update_rs_oop_cl);
-
-  FilterOutOfRegionClosure filter_then_update_rs_oop_cl(r,
-                        (check_for_refs_into_cset ?
-                                (OopClosure*)&mux :
-                                (OopClosure*)&update_rs_oop_cl));
-
   bool card_processed =
     r->oops_on_card_seq_iterate_careful(dirty_region,
-                                        &filter_then_update_rs_oop_cl);
+                                        &update_rs_oop_cl);
 
   // If unable to process the card then we encountered an unparsable
   // part of the heap (e.g. a partially allocated object) while
@@ -731,15 +711,15 @@
     _conc_refine_cards++;
   }
 
-  // This gets set to true if the card being refined has
-  // references that point into the collection set.
-  bool has_refs_into_cset = trigger_cl.triggered();
+  // This gets set to true if the card being refined has references that point
+  // into the collection set.
+  bool has_refs_into_cset = update_rs_oop_cl.has_refs_into_cset();
 
   // We should only be detecting that the card contains references
   // that point into the collection set if the current thread is
   // a GC worker thread.
   assert(!has_refs_into_cset || SafepointSynchronize::is_at_safepoint(),
-           "invalid result at non safepoint");
+         "invalid result at non safepoint");
 
   return has_refs_into_cset;
 }
--- a/hotspot/src/share/vm/gc/g1/g1_specialized_oop_closures.hpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/g1_specialized_oop_closures.hpp	Mon Mar 06 17:03:35 2017 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -35,14 +35,14 @@
 class G1ParScanClosure;
 class G1ParPushHeapRSClosure;
 
-class FilterOutOfRegionClosure;
+class G1UpdateRSOrPushRefOopClosure;
 class G1CMOopClosure;
 class G1RootRegionScanClosure;
 
 #define SPECIALIZED_OOP_OOP_ITERATE_CLOSURES_G1(f) \
       f(G1ParScanClosure,_nv)                      \
       f(G1ParPushHeapRSClosure,_nv)                \
-      f(FilterOutOfRegionClosure,_nv)              \
+      f(G1UpdateRSOrPushRefOopClosure,_nv)         \
       f(G1CMOopClosure,_nv)                        \
       f(G1RootRegionScanClosure,_nv)
 
--- a/hotspot/src/share/vm/gc/g1/heapRegion.cpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/heapRegion.cpp	Mon Mar 06 17:03:35 2017 +0100
@@ -55,10 +55,6 @@
   DirtyCardToOopClosure(hr, cl, precision, NULL),
   _hr(hr), _rs_scan(cl), _g1(g1) { }
 
-FilterOutOfRegionClosure::FilterOutOfRegionClosure(HeapRegion* r,
-                                                   OopClosure* oc) :
-  _r_bottom(r->bottom()), _r_end(r->end()), _oc(oc) { }
-
 void HeapRegionDCTOC::walk_mem_region(MemRegion mr,
                                       HeapWord* bottom,
                                       HeapWord* top) {
@@ -357,7 +353,7 @@
 // special handling for concurrent processing encountering an
 // in-progress allocation.
 static bool do_oops_on_card_in_humongous(MemRegion mr,
-                                         FilterOutOfRegionClosure* cl,
+                                         G1UpdateRSOrPushRefOopClosure* cl,
                                          HeapRegion* hr,
                                          G1CollectedHeap* g1h) {
   assert(hr->is_humongous(), "precondition");
@@ -398,7 +394,7 @@
 }
 
 bool HeapRegion::oops_on_card_seq_iterate_careful(MemRegion mr,
-                                                  FilterOutOfRegionClosure* cl) {
+                                                  G1UpdateRSOrPushRefOopClosure* cl) {
   assert(MemRegion(bottom(), end()).contains(mr), "Card region not in heap region");
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
 
@@ -769,6 +765,21 @@
   }
 };
 
+// Closure that applies the given two closures in sequence.
+class G1Mux2Closure : public OopClosure {
+  OopClosure* _c1;
+  OopClosure* _c2;
+public:
+  G1Mux2Closure(OopClosure *c1, OopClosure *c2) { _c1 = c1; _c2 = c2; }
+  template <class T> inline void do_oop_work(T* p) {
+    // Apply first closure; then apply the second.
+    _c1->do_oop(p);
+    _c2->do_oop(p);
+  }
+  virtual inline void do_oop(oop* p) { do_oop_work(p); }
+  virtual inline void do_oop(narrowOop* p) { do_oop_work(p); }
+};
+
 // This really ought to be commoned up into OffsetTableContigSpace somehow.
 // We would need a mechanism to make that code skip dead objects.
 
--- a/hotspot/src/share/vm/gc/g1/heapRegion.hpp	Mon Mar 06 15:33:14 2017 +0100
+++ b/hotspot/src/share/vm/gc/g1/heapRegion.hpp	Mon Mar 06 17:03:35 2017 +0100
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -661,7 +661,7 @@
   // encountered; that only happens when invoked concurrently with the
   // mutator.
   bool oops_on_card_seq_iterate_careful(MemRegion mr,
-                                        FilterOutOfRegionClosure* cl);
+                                        G1UpdateRSOrPushRefOopClosure* cl);
 
   size_t recorded_rs_length() const        { return _recorded_rs_length; }
   double predicted_elapsed_time_ms() const { return _predicted_elapsed_time_ms; }