6978187: G1: assert(ParallelGCThreads> 1 || n_yielded() == _hrrs->occupied()) strikes again

Summary: An evacuation failure while copying the roots caused an object, A, to be forwarded to itself. During the subsequent RSet updating a reference to A was processed causing the reference to be added to the RSet of A's heap region. As a result of adding to the remembered set we ran into the issue described in 6930581 - the sparse table expanded and the RSet scanning code walked the cards in one instance of RHashTable (_cur) while the occupied() counts the cards in the expanded table (_next).
Reviewed-by: tonyp, iveresov

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

@@ -795,6 +795,7 @@ public:
     _worker_i(worker_i),
     _g1h(g1)
   { }
+
   bool doHeapRegion(HeapRegion* r) {
     if (!r->continuesHumongous()) {
       _cl.set_from(r);

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

@@ -116,7 +116,6 @@ G1RemSet::G1RemSet(G1CollectedHeap* g1, CardTableModRefBS* ct_bs)
   : _g1(g1), _conc_refine_cards(0),
     _ct_bs(ct_bs), _g1p(_g1->g1_policy()),
     _cg1r(g1->concurrent_g1_refine()),
-    _traversal_in_progress(false),
     _cset_rs_update_cl(NULL),
     _cards_scanned(NULL), _total_cards_scanned(0)
 {
@@ -512,8 +511,6 @@ void G1RemSet::prepare_for_oops_into_collection_set_do() {
   DirtyCardQueueSet& dcqs = JavaThread::dirty_card_queue_set();
   dcqs.concatenate_logs();
 
-  assert(!_traversal_in_progress, "Invariant between iterations.");
-  set_traversal(true);
   if (ParallelGCThreads > 0) {
     _seq_task->set_n_threads((int)n_workers());
   }
@@ -539,9 +536,6 @@ class cleanUpIteratorsClosure : public HeapRegionClosure {
 // through the oops which coincide with that card. It scans the reference
 // fields in each oop; when it finds an oop that points into the collection
 // set, the RSet for the region containing the referenced object is updated.
-// Note: _par_traversal_in_progress in the G1RemSet must be FALSE; otherwise
-// the UpdateRSetImmediate closure will cause cards to be enqueued on to
-// the DCQS that we're iterating over, causing an infinite loop.
 class UpdateRSetCardTableEntryIntoCSetClosure: public CardTableEntryClosure {
   G1CollectedHeap* _g1;
   CardTableModRefBS* _ct_bs;
@@ -611,8 +605,6 @@ void G1RemSet::cleanup_after_oops_into_collection_set_do() {
   // Set all cards back to clean.
   _g1->cleanUpCardTable();
 
-  set_traversal(false);
-
   DirtyCardQueueSet& into_cset_dcqs = _g1->into_cset_dirty_card_queue_set();
   int into_cset_n_buffers = into_cset_dcqs.completed_buffers_num();
 
@@ -645,21 +637,8 @@ void G1RemSet::cleanup_after_oops_into_collection_set_do() {
   assert(_g1->into_cset_dirty_card_queue_set().completed_buffers_num() == 0,
          "all buffers should be freed");
   _g1->into_cset_dirty_card_queue_set().clear_n_completed_buffers();
-
-  assert(!_traversal_in_progress, "Invariant between iterations.");
 }
 
-class UpdateRSObjectClosure: public ObjectClosure {
-  UpdateRSOopClosure* _update_rs_oop_cl;
-public:
-  UpdateRSObjectClosure(UpdateRSOopClosure* update_rs_oop_cl) :
-    _update_rs_oop_cl(update_rs_oop_cl) {}
-  void do_object(oop obj) {
-    obj->oop_iterate(_update_rs_oop_cl);
-  }
-
-};
-
 class ScrubRSClosure: public HeapRegionClosure {
   G1CollectedHeap* _g1h;
   BitMap* _region_bm;
@@ -749,7 +728,12 @@ bool G1RemSet::concurrentRefineOneCard_impl(jbyte* card_ptr, int worker_i,
   ct_freq_note_card(_ct_bs->index_for(start));
 #endif
 
-  UpdateRSOopClosure update_rs_oop_cl(this, worker_i);
+  assert(!check_for_refs_into_cset || _cset_rs_update_cl[worker_i] != NULL, "sanity");
+  UpdateRSOrPushRefOopClosure update_rs_oop_cl(_g1,
+                                               _g1->g1_rem_set(),
+                                               _cset_rs_update_cl[worker_i],
+                                               check_for_refs_into_cset,
+                                               worker_i);
   update_rs_oop_cl.set_from(r);
 
   TriggerClosure trigger_cl;

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

@@ -59,11 +59,6 @@ protected:
   size_t*             _cards_scanned;
   size_t              _total_cards_scanned;
 
-  // _traversal_in_progress is "true" iff a traversal is in progress.
-
-  bool _traversal_in_progress;
-  void set_traversal(bool b) { _traversal_in_progress = b; }
-
   // Used for caching the closure that is responsible for scanning
   // references into the collection set.
   OopsInHeapRegionClosure** _cset_rs_update_cl;
@@ -76,10 +71,6 @@ protected:
   bool concurrentRefineOneCard_impl(jbyte* card_ptr, int worker_i,
                                     bool check_for_refs_into_cset);
 
-protected:
-  template <class T> void write_ref_nv(HeapRegion* from, T* p);
-  template <class T> void par_write_ref_nv(HeapRegion* from, T* p, int tid);
-
 public:
   // This is called to reset dual hash tables after the gc pause
   // is finished and the initial hash table is no longer being
@@ -117,22 +108,8 @@ public:
 
   // Record, if necessary, the fact that *p (where "p" is in region "from",
   // which is required to be non-NULL) has changed to a new non-NULL value.
-  // [Below the virtual version calls a non-virtual protected
-  // workhorse that is templatified for narrow vs wide oop.]
-  inline void write_ref(HeapRegion* from, oop* p) {
-    write_ref_nv(from, p);
-  }
-  inline void write_ref(HeapRegion* from, narrowOop* p) {
-    write_ref_nv(from, p);
-  }
-  inline void par_write_ref(HeapRegion* from, oop* p, int tid) {
-    par_write_ref_nv(from, p, tid);
-  }
-  inline void par_write_ref(HeapRegion* from, narrowOop* p, int tid) {
-    par_write_ref_nv(from, p, tid);
-  }
-
-  bool self_forwarded(oop obj);
+  template <class T> void write_ref(HeapRegion* from, T* p);
+  template <class T> void par_write_ref(HeapRegion* from, T* p, int tid);
 
   // Requires "region_bm" and "card_bm" to be bitmaps with 1 bit per region
   // or card, respectively, such that a region or card with a corresponding
@@ -186,9 +163,8 @@ class UpdateRSOopClosure: public OopClosure {
 
 public:
   UpdateRSOopClosure(G1RemSet* rs, int worker_i = 0) :
-    _from(NULL), _rs(rs), _worker_i(worker_i) {
-    guarantee(_rs != NULL, "Requires an HRIntoG1RemSet");
-  }
+    _from(NULL), _rs(rs), _worker_i(worker_i)
+  {}
 
   void set_from(HeapRegion* from) {
     assert(from != NULL, "from region must be non-NULL");
@@ -215,3 +191,43 @@ public:
   virtual void do_oop(narrowOop* p) { do_oop_work(p); }
   virtual void do_oop(      oop* p) { do_oop_work(p); }
 };
+
+class UpdateRSOrPushRefOopClosure: public OopClosure {
+  G1CollectedHeap* _g1;
+  G1RemSet* _g1_rem_set;
+  HeapRegion* _from;
+  OopsInHeapRegionClosure* _push_ref_cl;
+  bool _record_refs_into_cset;
+  int _worker_i;
+
+  template <class T> void do_oop_work(T* p);
+
+public:
+  UpdateRSOrPushRefOopClosure(G1CollectedHeap* g1h,
+                              G1RemSet* rs,
+                              OopsInHeapRegionClosure* push_ref_cl,
+                              bool record_refs_into_cset,
+                              int worker_i = 0) :
+    _g1(g1h),
+    _g1_rem_set(rs),
+    _from(NULL),
+    _record_refs_into_cset(record_refs_into_cset),
+    _push_ref_cl(push_ref_cl),
+    _worker_i(worker_i) { }
+
+  void set_from(HeapRegion* from) {
+    assert(from != NULL, "from region must be non-NULL");
+    _from = from;
+  }
+
+  bool self_forwarded(oop obj) {
+    bool result = (obj->is_forwarded() && (obj->forwardee()== obj));
+    return result;
+  }
+
+  virtual void do_oop(narrowOop* p) { do_oop_work(p); }
+  virtual void do_oop(oop* p)       { do_oop_work(p); }
+
+  bool apply_to_weak_ref_discovered_field() { return true; }
+};
+

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

@@ -31,17 +31,12 @@ inline size_t G1RemSet::n_workers() {
 }
 
 template <class T>
-inline void G1RemSet::write_ref_nv(HeapRegion* from, T* p) {
-  par_write_ref_nv(from, p, 0);
-}
-
-inline bool G1RemSet::self_forwarded(oop obj) {
-  bool result =  (obj->is_forwarded() && (obj->forwardee()== obj));
-  return result;
+inline void G1RemSet::write_ref(HeapRegion* from, T* p) {
+  par_write_ref(from, p, 0);
 }
 
 template <class T>
-inline void G1RemSet::par_write_ref_nv(HeapRegion* from, T* p, int tid) {
+inline void G1RemSet::par_write_ref(HeapRegion* from, T* p, int tid) {
   oop obj = oopDesc::load_decode_heap_oop(p);
 #ifdef ASSERT
   // can't do because of races
@@ -62,25 +57,7 @@ inline void G1RemSet::par_write_ref_nv(HeapRegion* from, T* p, int tid) {
   assert(from == NULL || from->is_in_reserved(p), "p is not in from");
 
   HeapRegion* to = _g1->heap_region_containing(obj);
-  // The test below could be optimized by applying a bit op to to and from.
-  if (to != NULL && from != NULL && from != to) {
-    // The _traversal_in_progress flag is true during the collection pause,
-    // false during the evacuation failure handling. This should avoid a
-    // potential loop if we were to add the card containing 'p' to the DCQS
-    // that's used to regenerate the remembered sets for the collection set,
-    // in the event of an evacuation failure, here. The UpdateRSImmediate
-    // closure will eventally call this routine.
-    if (_traversal_in_progress &&
-        to->in_collection_set() && !self_forwarded(obj)) {
-
-      assert(_cset_rs_update_cl[tid] != NULL, "should have been set already");
-      _cset_rs_update_cl[tid]->do_oop(p);
-
-      // Deferred updates to the CSet are either discarded (in the normal case),
-      // or processed (if an evacuation failure occurs) at the end
-      // of the collection.
-      // See G1RemSet::cleanup_after_oops_into_collection_set_do().
-    } else {
+  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 ")",
@@ -90,7 +67,6 @@ inline void G1RemSet::par_write_ref_nv(HeapRegion* from, T* p, int tid) {
     assert(to->rem_set() != NULL, "Need per-region 'into' remsets.");
     to->rem_set()->add_reference(p, tid);
   }
-  }
 }
 
 template <class T>
@@ -108,3 +84,64 @@ inline void UpdateRSetImmediate::do_oop_work(T* p) {
   }
 }
 
+template <class T>
+inline void UpdateRSOrPushRefOopClosure::do_oop_work(T* p) {
+  oop obj = oopDesc::load_decode_heap_oop(p);
+#ifdef ASSERT
+  // can't do because of races
+  // assert(obj == NULL || obj->is_oop(), "expected an oop");
+
+  // Do the safe subset of is_oop
+  if (obj != NULL) {
+#ifdef CHECK_UNHANDLED_OOPS
+    oopDesc* o = obj.obj();
+#else
+    oopDesc* o = obj;
+#endif // CHECK_UNHANDLED_OOPS
+    assert((intptr_t)o % MinObjAlignmentInBytes == 0, "not oop aligned");
+    assert(Universe::heap()->is_in_reserved(obj), "must be in heap");
+  }
+#endif // ASSERT
+
+  assert(_from != NULL, "from region must be non-NULL");
+
+  HeapRegion* to = _g1->heap_region_containing(obj);
+  if (to != NULL && _from != to) {
+    // The _record_refs_into_cset flag is true during the RSet
+    // updating part of an evacuation pause. It is false at all
+    // other times:
+    //  * rebuilding the rembered sets after a full GC
+    //  * during concurrent refinement.
+    //  * updating the remembered sets of regions in the collection
+    //    set in the event of an evacuation failure (when deferred
+    //    updates are enabled).
+
+    if (_record_refs_into_cset && to->in_collection_set()) {
+      // We are recording references that point into the collection
+      // set and this particular reference does exactly that...
+      // If the referenced object has already been forwarded
+      // to itself, we are handling an evacuation failure and
+      // we have already visited/tried to copy this object
+      // there is no need to retry.
+      if (!self_forwarded(obj)) {
+        assert(_push_ref_cl != NULL, "should not be null");
+        // Push the reference in the refs queue of the G1ParScanThreadState
+        // instance for this worker thread.
+        _push_ref_cl->do_oop(p);
+      }
+
+      // Deferred updates to the CSet are either discarded (in the normal case),
+      // or processed (if an evacuation failure occurs) at the end
+      // of the collection.
+      // See G1RemSet::cleanup_after_oops_into_collection_set_do().
+    } else {
+      // We either don't care about pushing references that point into the
+      // collection set (i.e. we're not during an evacuation pause) _or_
+      // the reference doesn't point into the collection set. Either way
+      // we add the reference directly to the RSet of the region containing
+      // the referenced object.
+      _g1_rem_set->par_write_ref(_from, p, _worker_i);
+    }
+  }
+}
+