7046182: G1: remove unnecessary iterations over the collection set

Remove two unnecessary iterations over the collection set which are supposed to prepare the RSet's of the CSet regions for parallel iterations (we'll make sure this is done incrementally). I'll piggyback on this CR the removal of the G1_REM_SET_LOGGING code.

Reviewed-by: brutisso, johnc

hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp

@@ -3015,6 +3015,56 @@ void G1CollectedHeap::print_tracing_info() const {
   SpecializationStats::print();
 }
 
+#ifndef PRODUCT
+// Helpful for debugging RSet issues.
+
+class PrintRSetsClosure : public HeapRegionClosure {
+private:
+  const char* _msg;
+  size_t _occupied_sum;
+
+public:
+  bool doHeapRegion(HeapRegion* r) {
+    HeapRegionRemSet* hrrs = r->rem_set();
+    size_t occupied = hrrs->occupied();
+    _occupied_sum += occupied;
+
+    gclog_or_tty->print_cr("Printing RSet for region "HR_FORMAT,
+                           HR_FORMAT_PARAMS(r));
+    if (occupied == 0) {
+      gclog_or_tty->print_cr("  RSet is empty");
+    } else {
+      hrrs->print();
+    }
+    gclog_or_tty->print_cr("----------");
+    return false;
+  }
+
+  PrintRSetsClosure(const char* msg) : _msg(msg), _occupied_sum(0) {
+    gclog_or_tty->cr();
+    gclog_or_tty->print_cr("========================================");
+    gclog_or_tty->print_cr(msg);
+    gclog_or_tty->cr();
+  }
+
+  ~PrintRSetsClosure() {
+    gclog_or_tty->print_cr("Occupied Sum: "SIZE_FORMAT, _occupied_sum);
+    gclog_or_tty->print_cr("========================================");
+    gclog_or_tty->cr();
+  }
+};
+
+void G1CollectedHeap::print_cset_rsets() {
+  PrintRSetsClosure cl("Printing CSet RSets");
+  collection_set_iterate(&cl);
+}
+
+void G1CollectedHeap::print_all_rsets() {
+  PrintRSetsClosure cl("Printing All RSets");;
+  heap_region_iterate(&cl);
+}
+#endif // PRODUCT
+
 G1CollectedHeap* G1CollectedHeap::heap() {
   assert(_sh->kind() == CollectedHeap::G1CollectedHeap,
          "not a garbage-first heap");
@@ -3148,12 +3198,27 @@ G1CollectedHeap::cleanup_surviving_young_words() {
 
 // </NEW PREDICTION>
 
-struct PrepareForRSScanningClosure : public HeapRegionClosure {
-  bool doHeapRegion(HeapRegion *r) {
-    r->rem_set()->set_iter_claimed(0);
+#ifdef ASSERT
+class VerifyCSetClosure: public HeapRegionClosure {
+public:
+  bool doHeapRegion(HeapRegion* hr) {
+    // Here we check that the CSet region's RSet is ready for parallel
+    // iteration. The fields that we'll verify are only manipulated
+    // when the region is part of a CSet and is collected. Afterwards,
+    // we reset these fields when we clear the region's RSet (when the
+    // region is freed) so they are ready when the region is
+    // re-allocated. The only exception to this is if there's an
+    // evacuation failure and instead of freeing the region we leave
+    // it in the heap. In that case, we reset these fields during
+    // evacuation failure handling.
+    guarantee(hr->rem_set()->verify_ready_for_par_iteration(), "verification");
+
+    // Here's a good place to add any other checks we'd like to
+    // perform on CSet regions.
     return false;
   }
 };
+#endif // ASSERT
 
 #if TASKQUEUE_STATS
 void G1CollectedHeap::print_taskqueue_stats_hdr(outputStream* const st) {
@@ -3257,11 +3322,6 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) {
       gc_prologue(false);
       increment_total_collections(false /* full gc */);
 
-#if G1_REM_SET_LOGGING
-      gclog_or_tty->print_cr("\nJust chose CS, heap:");
-      print();
-#endif
-
       if (VerifyBeforeGC && total_collections() >= VerifyGCStartAt) {
         HandleMark hm;  // Discard invalid handles created during verification
         gclog_or_tty->print(" VerifyBeforeGC:");
@@ -3347,13 +3407,10 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) {
         concurrent_mark()->reset_active_task_region_fields_in_cset();
       }
 
-      // Nothing to do if we were unable to choose a collection set.
-#if G1_REM_SET_LOGGING
-      gclog_or_tty->print_cr("\nAfter pause, heap:");
-      print();
-#endif
-      PrepareForRSScanningClosure prepare_for_rs_scan;
-      collection_set_iterate(&prepare_for_rs_scan);
+#ifdef ASSERT
+      VerifyCSetClosure cl;
+      collection_set_iterate(&cl);
+#endif // ASSERT
 
       setup_surviving_young_words();
 
@@ -3955,6 +4012,14 @@ void G1CollectedHeap::remove_self_forwarding_pointers() {
       assert(cur->in_collection_set(), "bad CS");
       RemoveSelfPointerClosure rspc(_g1h, cur, cl);
 
+      // In the common case we make sure that this is done when the
+      // region is freed so that it is "ready-to-go" when it's
+      // re-allocated. However, when evacuation failure happens, a
+      // region will remain in the heap and might ultimately be added
+      // to a CSet in the future. So we have to be careful here and
+      // make sure the region's RSet is ready for parallel iteration
+      // whenever this might be required in the future.
+      cur->rem_set()->reset_for_par_iteration();
       cur->reset_bot();
       cl->set_region(cur);
       cur->object_iterate(&rspc);
@@ -4474,10 +4539,6 @@ void G1ParCopyClosure <do_gen_barrier, barrier, do_mark_forwardee>
 
   // here the null check is implicit in the cset_fast_test() test
   if (_g1->in_cset_fast_test(obj)) {
-#if G1_REM_SET_LOGGING
-    gclog_or_tty->print_cr("Loc "PTR_FORMAT" contains pointer "PTR_FORMAT" "
-                           "into CS.", p, (void*) obj);
-#endif
     if (obj->is_forwarded()) {
       oopDesc::encode_store_heap_oop(p, obj->forwardee());
     } else {

hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp

@@ -1372,6 +1372,10 @@ public:
   // Override
   void print_tracing_info() const;
 
+  // The following two methods are helpful for debugging RSet issues.
+  void print_cset_rsets() PRODUCT_RETURN;
+  void print_all_rsets() PRODUCT_RETURN;
+
   // Convenience function to be used in situations where the heap type can be
   // asserted to be this type.
   static G1CollectedHeap* heap();

hotspot/src/share/vm/gc_implementation/g1/g1RemSet.cpp

@@ -297,31 +297,6 @@ void G1RemSet::updateRS(DirtyCardQueue* into_cset_dcq, int worker_i) {
   _g1p->record_update_rs_time(worker_i, (os::elapsedTime() - start) * 1000.0);
 }
 
-#ifndef PRODUCT
-class PrintRSClosure : public HeapRegionClosure {
-  int _count;
-public:
-  PrintRSClosure() : _count(0) {}
-  bool doHeapRegion(HeapRegion* r) {
-    HeapRegionRemSet* hrrs = r->rem_set();
-    _count += (int) hrrs->occupied();
-    if (hrrs->occupied() == 0) {
-      gclog_or_tty->print("Heap Region [" PTR_FORMAT ", " PTR_FORMAT ") "
-                          "has no remset entries\n",
-                          r->bottom(), r->end());
-    } else {
-      gclog_or_tty->print("Printing rem set for heap region [" PTR_FORMAT ", " PTR_FORMAT ")\n",
-                          r->bottom(), r->end());
-      r->print();
-      hrrs->print();
-      gclog_or_tty->print("\nDone printing rem set\n");
-    }
-    return false;
-  }
-  int occupied() {return _count;}
-};
-#endif
-
 class CountRSSizeClosure: public HeapRegionClosure {
   size_t _n;
   size_t _tot;
@@ -447,10 +422,6 @@ void G1RemSet::oops_into_collection_set_do(OopsInHeapRegionClosure* oc,
 }
 
 void G1RemSet::prepare_for_oops_into_collection_set_do() {
-#if G1_REM_SET_LOGGING
-  PrintRSClosure cl;
-  _g1->collection_set_iterate(&cl);
-#endif
   cleanupHRRS();
   ConcurrentG1Refine* cg1r = _g1->concurrent_g1_refine();
   _g1->set_refine_cte_cl_concurrency(false);
@@ -469,14 +440,6 @@ void G1RemSet::prepare_for_oops_into_collection_set_do() {
 }
 
 
-class cleanUpIteratorsClosure : public HeapRegionClosure {
-  bool doHeapRegion(HeapRegion *r) {
-    HeapRegionRemSet* hrrs = r->rem_set();
-    hrrs->init_for_par_iteration();
-    return false;
-  }
-};
-
 // This closure, applied to a DirtyCardQueueSet, is used to immediately
 // update the RSets for the regions in the CSet. For each card it iterates
 // through the oops which coincide with that card. It scans the reference
@@ -537,18 +500,13 @@ public:
 void G1RemSet::cleanup_after_oops_into_collection_set_do() {
   guarantee( _cards_scanned != NULL, "invariant" );
   _total_cards_scanned = 0;
-  for (uint i = 0; i < n_workers(); ++i)
+  for (uint i = 0; i < n_workers(); ++i) {
     _total_cards_scanned += _cards_scanned[i];
+  }
   FREE_C_HEAP_ARRAY(size_t, _cards_scanned);
   _cards_scanned = NULL;
   // Cleanup after copy
-#if G1_REM_SET_LOGGING
-  PrintRSClosure cl;
-  _g1->heap_region_iterate(&cl);
-#endif
   _g1->set_refine_cte_cl_concurrency(true);
-  cleanUpIteratorsClosure iterClosure;
-  _g1->collection_set_iterate(&iterClosure);
   // Set all cards back to clean.
   _g1->cleanUpCardTable();
 

hotspot/src/share/vm/gc_implementation/g1/g1RemSet.hpp

@@ -142,8 +142,6 @@ public:
   virtual void prepare_for_verify();
 };
 
-#define G1_REM_SET_LOGGING 0
-
 class CountNonCleanMemRegionClosure: public MemRegionClosure {
   G1CollectedHeap* _g1;
   int _n;

hotspot/src/share/vm/gc_implementation/g1/g1RemSet.inline.hpp

@@ -65,12 +65,6 @@ inline void G1RemSet::par_write_ref(HeapRegion* from, T* p, int tid) {
 
   HeapRegion* to = _g1->heap_region_containing(obj);
   if (to != NULL && from != to) {
-#if G1_REM_SET_LOGGING
-    gclog_or_tty->print_cr("Adding " PTR_FORMAT " (" PTR_FORMAT ") to RS"
-                           " for region [" PTR_FORMAT ", " PTR_FORMAT ")",
-                           p, obj,
-                           to->bottom(), to->end());
-#endif
     assert(to->rem_set() != NULL, "Need per-region 'into' remsets.");
     to->rem_set()->add_reference(p, tid);
   }

hotspot/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp

@@ -1085,8 +1085,9 @@ int HeapRegionRemSet::num_par_rem_sets() {
 
 HeapRegionRemSet::HeapRegionRemSet(G1BlockOffsetSharedArray* bosa,
                                    HeapRegion* hr)
-  : _bosa(bosa), _other_regions(hr), _iter_state(Unclaimed) { }
-
+  : _bosa(bosa), _other_regions(hr) {
+  reset_for_par_iteration();
+}
 
 void HeapRegionRemSet::setup_remset_size() {
   // Setup sparse and fine-grain tables sizes.
@@ -1101,10 +1102,6 @@ void HeapRegionRemSet::setup_remset_size() {
   guarantee(G1RSetSparseRegionEntries > 0 && G1RSetRegionEntries > 0 , "Sanity");
 }
 
-void HeapRegionRemSet::init_for_par_iteration() {
-  _iter_state = Unclaimed;
-}
-
 bool HeapRegionRemSet::claim_iter() {
   if (_iter_state != Unclaimed) return false;
   jint res = Atomic::cmpxchg(Claimed, (jint*)(&_iter_state), Unclaimed);
@@ -1119,7 +1116,6 @@ bool HeapRegionRemSet::iter_is_complete() {
   return _iter_state == Complete;
 }
 
-
 void HeapRegionRemSet::init_iterator(HeapRegionRemSetIterator* iter) const {
   iter->initialize(this);
 }
@@ -1132,7 +1128,7 @@ void HeapRegionRemSet::print() const {
   while (iter.has_next(card_index)) {
     HeapWord* card_start =
       G1CollectedHeap::heap()->bot_shared()->address_for_index(card_index);
-    gclog_or_tty->print_cr("  Card " PTR_FORMAT ".", card_start);
+    gclog_or_tty->print_cr("  Card " PTR_FORMAT, card_start);
   }
   // XXX
   if (iter.n_yielded() != occupied()) {
@@ -1159,6 +1155,14 @@ void HeapRegionRemSet::par_cleanup() {
 void HeapRegionRemSet::clear() {
   _other_regions.clear();
   assert(occupied() == 0, "Should be clear.");
+  reset_for_par_iteration();
+}
+
+void HeapRegionRemSet::reset_for_par_iteration() {
+  _iter_state = Unclaimed;
+  _iter_claimed = 0;
+  // It's good to check this to make sure that the two methods are in sync.
+  assert(verify_ready_for_par_iteration(), "post-condition");
 }
 
 void HeapRegionRemSet::scrub(CardTableModRefBS* ctbs,

hotspot/src/share/vm/gc_implementation/g1/heapRegionRemSet.hpp

@@ -262,8 +262,6 @@ public:
   virtual void cleanup() = 0;
 #endif
 
-  // Should be called from single-threaded code.
-  void init_for_par_iteration();
   // Attempt to claim the region.  Returns true iff this call caused an
   // atomic transition from Unclaimed to Claimed.
   bool claim_iter();
@@ -273,7 +271,6 @@ public:
   bool iter_is_complete();
 
   // Support for claiming blocks of cards during iteration
-  void set_iter_claimed(size_t x) { _iter_claimed = (jlong)x; }
   size_t iter_claimed() const { return (size_t)_iter_claimed; }
   // Claim the next block of cards
   size_t iter_claimed_next(size_t step) {
@@ -284,6 +281,11 @@ public:
     } while (Atomic::cmpxchg((jlong)next, &_iter_claimed, (jlong)current) != (jlong)current);
     return current;
   }
+  void reset_for_par_iteration();
+
+  bool verify_ready_for_par_iteration() {
+    return (_iter_state == Unclaimed) && (_iter_claimed == 0);
+  }
 
   // Initialize the given iterator to iterate over this rem set.
   void init_iterator(HeapRegionRemSetIterator* iter) const;

hotspot/src/share/vm/gc_implementation/g1/heapRegionSets.cpp

@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "gc_implementation/g1/heapRegionRemSet.hpp"
 #include "gc_implementation/g1/heapRegionSets.hpp"
 
 //////////////////// FreeRegionList ////////////////////
@@ -38,6 +39,16 @@ const char* FreeRegionList::verify_region_extra(HeapRegion* hr) {
 
 //////////////////// MasterFreeRegionList ////////////////////
 
+const char* MasterFreeRegionList::verify_region_extra(HeapRegion* hr) {
+  // We should reset the RSet for parallel iteration before we add it
+  // to the master free list so that it is ready when the region is
+  // re-allocated.
+  if (!hr->rem_set()->verify_ready_for_par_iteration()) {
+    return "the region's RSet should be ready for parallel iteration";
+  }
+  return FreeRegionList::verify_region_extra(hr);
+}
+
 bool MasterFreeRegionList::check_mt_safety() {
   // Master Free List MT safety protocol:
   // (a) If we're at a safepoint, operations on the master free list

hotspot/src/share/vm/gc_implementation/g1/heapRegionSets.hpp

@@ -44,6 +44,7 @@ public:
 
 class MasterFreeRegionList : public FreeRegionList {
 protected:
+  virtual const char* verify_region_extra(HeapRegion* hr);
   virtual bool check_mt_safety();
 
 public: