--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp	Tue Jun 21 15:23:07 2011 -0400
@@ -3015,6 +3015,56 @@
   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 @@
 
 // </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 @@
       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 @@
         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 @@
       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 @@
 
   // 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 {

--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp	Tue Jun 21 15:23:07 2011 -0400
@@ -1372,6 +1372,10 @@
   // 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();

--- a/src/share/vm/gc_implementation/g1/g1RemSet.cpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/g1RemSet.cpp	Tue Jun 21 15:23:07 2011 -0400
@@ -297,31 +297,6 @@
   _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::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 @@
 }
 
 
-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 @@
 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();
 

--- a/src/share/vm/gc_implementation/g1/g1RemSet.hpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/g1RemSet.hpp	Tue Jun 21 15:23:07 2011 -0400
@@ -142,8 +142,6 @@
   virtual void prepare_for_verify();
 };
 
-#define G1_REM_SET_LOGGING 0
-
 class CountNonCleanMemRegionClosure: public MemRegionClosure {
   G1CollectedHeap* _g1;
   int _n;

--- a/src/share/vm/gc_implementation/g1/g1RemSet.inline.hpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/g1RemSet.inline.hpp	Tue Jun 21 15:23:07 2011 -0400
@@ -65,12 +65,6 @@
 
   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);
   }

--- a/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp	Tue Jun 21 15:23:07 2011 -0400
@@ -1085,8 +1085,9 @@
 
 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 @@
   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 @@
   return _iter_state == Complete;
 }
 
-
 void HeapRegionRemSet::init_iterator(HeapRegionRemSetIterator* iter) const {
   iter->initialize(this);
 }
@@ -1132,7 +1128,7 @@
   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::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,

--- a/src/share/vm/gc_implementation/g1/heapRegionRemSet.hpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/heapRegionRemSet.hpp	Tue Jun 21 15:23:07 2011 -0400
@@ -262,8 +262,6 @@
   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 @@
   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 @@
     } 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;

--- a/src/share/vm/gc_implementation/g1/heapRegionSets.cpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/heapRegionSets.cpp	Tue Jun 21 15:23:07 2011 -0400
@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "gc_implementation/g1/heapRegionRemSet.hpp"
 #include "gc_implementation/g1/heapRegionSets.hpp"
 
 //////////////////// FreeRegionList ////////////////////
@@ -38,6 +39,16 @@
 
 //////////////////// 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

--- a/src/share/vm/gc_implementation/g1/heapRegionSets.hpp	Mon Jun 20 22:03:13 2011 -0400
+++ b/src/share/vm/gc_implementation/g1/heapRegionSets.hpp	Tue Jun 21 15:23:07 2011 -0400
@@ -44,6 +44,7 @@
 
 class MasterFreeRegionList : public FreeRegionList {
 protected:
+  virtual const char* verify_region_extra(HeapRegion* hr);
   virtual bool check_mt_safety();
 
 public: