8052172: Evacuation failure handling in G1 does not evacuate all objects if...

8052172: Evacuation failure handling in G1 does not evacuate all objects if -XX:-G1DeferredRSUpdate is set
Summary: Remove -XX:-G1DeferredRSUpdate functionality as it is racy. During evacuation failure handling, threads where evacuation failure handling occurred may try to add remembered sets to regions which remembered sets are currently being scanned. The iterator to handle the remembered set scan does not support addition of entries during scan and so may skip valid references.
Reviewed-by: iveresov, brutisso, mgerdin

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

@@ -1483,9 +1483,7 @@ bool G1CollectedHeap::do_collection(bool explicit_gc,
 
       // Discard all rset updates
       JavaThread::dirty_card_queue_set().abandon_logs();
-      assert(!G1DeferredRSUpdate
-             || (G1DeferredRSUpdate &&
-                (dirty_card_queue_set().completed_buffers_num() == 0)), "Should not be any");
+      assert(dirty_card_queue_set().completed_buffers_num() == 0, "DCQS should be empty");
 
       _young_list->reset_sampled_info();
       // At this point there should be no regions in the
@@ -2100,15 +2098,13 @@ jint G1CollectedHeap::initialize() {
                                                 concurrent_g1_refine()->red_zone(),
                                                 Shared_DirtyCardQ_lock);
 
-  if (G1DeferredRSUpdate) {
-    dirty_card_queue_set().initialize(NULL, // Should never be called by the Java code
-                                      DirtyCardQ_CBL_mon,
-                                      DirtyCardQ_FL_lock,
-                                      -1, // never trigger processing
-                                      -1, // no limit on length
-                                      Shared_DirtyCardQ_lock,
-                                      &JavaThread::dirty_card_queue_set());
-  }
+  dirty_card_queue_set().initialize(NULL, // Should never be called by the Java code
+                                    DirtyCardQ_CBL_mon,
+                                    DirtyCardQ_FL_lock,
+                                    -1, // never trigger processing
+                                    -1, // no limit on length
+                                    Shared_DirtyCardQ_lock,
+                                    &JavaThread::dirty_card_queue_set());
 
   // Initialize the card queue set used to hold cards containing
   // references into the collection set.
@@ -5303,7 +5299,6 @@ class G1RedirtyLoggedCardsTask : public AbstractGangTask {
 };
 
 void G1CollectedHeap::redirty_logged_cards() {
-  guarantee(G1DeferredRSUpdate, "Must only be called when using deferred RS updates.");
   double redirty_logged_cards_start = os::elapsedTime();
 
   uint n_workers = (G1CollectedHeap::use_parallel_gc_threads() ?
@@ -5963,9 +5958,7 @@ void G1CollectedHeap::evacuate_collection_set(EvacuationInfo& evacuation_info) {
   // RSets.
   enqueue_discovered_references(n_workers);
 
-  if (G1DeferredRSUpdate) {
-    redirty_logged_cards();
-  }
+  redirty_logged_cards();
   COMPILER2_PRESENT(DerivedPointerTable::update_pointers());
 }
 

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

@@ -176,15 +176,17 @@ public:
 class RemoveSelfForwardPtrHRClosure: public HeapRegionClosure {
   G1CollectedHeap* _g1h;
   ConcurrentMark* _cm;
-  OopsInHeapRegionClosure *_update_rset_cl;
   uint _worker_id;
 
+  DirtyCardQueue _dcq;
+  UpdateRSetDeferred _update_rset_cl;
+
 public:
   RemoveSelfForwardPtrHRClosure(G1CollectedHeap* g1h,
-                                OopsInHeapRegionClosure* update_rset_cl,
                                 uint worker_id) :
-    _g1h(g1h), _update_rset_cl(update_rset_cl),
-    _worker_id(worker_id), _cm(_g1h->concurrent_mark()) { }
+    _g1h(g1h), _dcq(&g1h->dirty_card_queue_set()), _update_rset_cl(g1h, &_dcq),
+    _worker_id(worker_id), _cm(_g1h->concurrent_mark()) {
+    }
 
   bool doHeapRegion(HeapRegion *hr) {
     bool during_initial_mark = _g1h->g1_policy()->during_initial_mark_pause();
@@ -195,7 +197,7 @@ public:
 
     if (hr->claimHeapRegion(HeapRegion::ParEvacFailureClaimValue)) {
       if (hr->evacuation_failed()) {
-        RemoveSelfForwardPtrObjClosure rspc(_g1h, _cm, hr, _update_rset_cl,
+        RemoveSelfForwardPtrObjClosure rspc(_g1h, _cm, hr, &_update_rset_cl,
                                             during_initial_mark,
                                             during_conc_mark,
                                             _worker_id);
@@ -214,7 +216,7 @@ public:
         // whenever this might be required in the future.
         hr->rem_set()->reset_for_par_iteration();
         hr->reset_bot();
-        _update_rset_cl->set_region(hr);
+        _update_rset_cl.set_region(hr);
         hr->object_iterate(&rspc);
 
         hr->rem_set()->clean_strong_code_roots(hr);
@@ -238,16 +240,7 @@ public:
     _g1h(g1h) { }
 
   void work(uint worker_id) {
-    UpdateRSetImmediate immediate_update(_g1h->g1_rem_set());
-    DirtyCardQueue dcq(&_g1h->dirty_card_queue_set());
-    UpdateRSetDeferred deferred_update(_g1h, &dcq);
-
-    OopsInHeapRegionClosure *update_rset_cl = &deferred_update;
-    if (!G1DeferredRSUpdate) {
-      update_rset_cl = &immediate_update;
-    }
-
-    RemoveSelfForwardPtrHRClosure rsfp_cl(_g1h, update_rset_cl, worker_id);
+    RemoveSelfForwardPtrHRClosure rsfp_cl(_g1h, worker_id);
 
     HeapRegion* hr = _g1h->start_cset_region_for_worker(worker_id);
     _g1h->collection_set_iterate_from(hr, &rsfp_cl);

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

@@ -236,10 +236,8 @@ void G1GCPhaseTimes::note_gc_end() {
   _last_gc_worker_times_ms.verify();
   _last_gc_worker_other_times_ms.verify();
 
-  if (G1DeferredRSUpdate) {
-    _last_redirty_logged_cards_time_ms.verify();
-    _last_redirty_logged_cards_processed_cards.verify();
-  }
+  _last_redirty_logged_cards_time_ms.verify();
+  _last_redirty_logged_cards_processed_cards.verify();
 }
 
 void G1GCPhaseTimes::note_string_dedup_fixup_start() {
@@ -351,12 +349,10 @@ void G1GCPhaseTimes::print(double pause_time_sec) {
     _recorded_non_young_cset_choice_time_ms));
   print_stats(2, "Ref Proc", _cur_ref_proc_time_ms);
   print_stats(2, "Ref Enq", _cur_ref_enq_time_ms);
-  if (G1DeferredRSUpdate) {
-    print_stats(2, "Redirty Cards", _recorded_redirty_logged_cards_time_ms);
-    if (G1Log::finest()) {
-      _last_redirty_logged_cards_time_ms.print(3, "Parallel Redirty");
-      _last_redirty_logged_cards_processed_cards.print(3, "Redirtied Cards");
-    }
+  print_stats(2, "Redirty Cards", _recorded_redirty_logged_cards_time_ms);
+  if (G1Log::finest()) {
+    _last_redirty_logged_cards_time_ms.print(3, "Parallel Redirty");
+    _last_redirty_logged_cards_processed_cards.print(3, "Redirtied Cards");
   }
   if (G1ReclaimDeadHumongousObjectsAtYoungGC) {
     print_stats(2, "Humongous Reclaim", _cur_fast_reclaim_humongous_time_ms);

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

@@ -82,20 +82,6 @@ class G1ParScanThreadState : public StackObj {
   DirtyCardQueue& dirty_card_queue()             { return _dcq;  }
   G1SATBCardTableModRefBS* ctbs()                { return _ct_bs; }
 
-  template <class T> inline void immediate_rs_update(HeapRegion* from, T* p, int tid);
-
-  template <class T> void deferred_rs_update(HeapRegion* from, T* p, int tid) {
-    // If the new value of the field points to the same region or
-    // is the to-space, we don't need to include it in the Rset updates.
-    if (!from->is_in_reserved(oopDesc::load_decode_heap_oop(p)) && !from->is_survivor()) {
-      size_t card_index = ctbs()->index_for(p);
-      // If the card hasn't been added to the buffer, do it.
-      if (ctbs()->mark_card_deferred(card_index)) {
-        dirty_card_queue().enqueue((jbyte*)ctbs()->byte_for_index(card_index));
-      }
-    }
-  }
-
  public:
   G1ParScanThreadState(G1CollectedHeap* g1h, uint queue_num, ReferenceProcessor* rp);
   ~G1ParScanThreadState();
@@ -115,8 +101,17 @@ class G1ParScanThreadState : public StackObj {
     _refs->push(ref);
   }
 
-  template <class T> inline void update_rs(HeapRegion* from, T* p, int tid);
-
+  template <class T> void update_rs(HeapRegion* from, T* p, int tid) {
+    // If the new value of the field points to the same region or
+    // is the to-space, we don't need to include it in the Rset updates.
+    if (!from->is_in_reserved(oopDesc::load_decode_heap_oop(p)) && !from->is_survivor()) {
+      size_t card_index = ctbs()->index_for(p);
+      // If the card hasn't been added to the buffer, do it.
+      if (ctbs()->mark_card_deferred(card_index)) {
+        dirty_card_queue().enqueue((jbyte*)ctbs()->byte_for_index(card_index));
+      }
+   }
+  }
  public:
 
   void set_evac_failure_closure(OopsInHeapRegionClosure* evac_failure_cl) {

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

@@ -29,20 +29,6 @@
 #include "gc_implementation/g1/g1RemSet.inline.hpp"
 #include "oops/oop.inline.hpp"
 
-template <class T> inline void G1ParScanThreadState::immediate_rs_update(HeapRegion* from, T* p, int tid) {
-  if (!from->is_survivor()) {
-    _g1_rem->par_write_ref(from, p, tid);
-  }
-}
-
-template <class T> void G1ParScanThreadState::update_rs(HeapRegion* from, T* p, int tid) {
-  if (G1DeferredRSUpdate) {
-    deferred_rs_update(from, p, tid);
-  } else {
-    immediate_rs_update(from, p, tid);
-  }
-}
-
 template <class T> void G1ParScanThreadState::do_oop_evac(T* p, HeapRegion* from) {
   assert(!oopDesc::is_null(oopDesc::load_decode_heap_oop(p)),
          "Reference should not be NULL here as such are never pushed to the task queue.");

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

@@ -338,12 +338,8 @@ void G1RemSet::oops_into_collection_set_do(OopsInHeapRegionClosure* oc,
   // are just discarded (there's no need to update the RSets of regions
   // that were in the collection set - after the pause these regions
   // are wholly 'free' of live objects. In the event of an evacuation
-  // failure the cards/buffers in this queue set are:
-  // * passed to the DirtyCardQueueSet that is used to manage deferred
-  //   RSet updates, or
-  // * scanned for references that point into the collection set
-  //   and the RSet of the corresponding region in the collection set
-  //   is updated immediately.
+  // failure the cards/buffers in this queue set are passed to the
+  // DirtyCardQueueSet that is used to manage RSet updates
   DirtyCardQueue into_cset_dcq(&_g1->into_cset_dirty_card_queue_set());
 
   assert((ParallelGCThreads > 0) || worker_i == 0, "invariant");
@@ -372,7 +368,6 @@ void G1RemSet::oops_into_collection_set_do(OopsInHeapRegionClosure* oc,
 
 void G1RemSet::prepare_for_oops_into_collection_set_do() {
   cleanupHRRS();
-  ConcurrentG1Refine* cg1r = _g1->concurrent_g1_refine();
   _g1->set_refine_cte_cl_concurrency(false);
   DirtyCardQueueSet& dcqs = JavaThread::dirty_card_queue_set();
   dcqs.concatenate_logs();
@@ -385,66 +380,6 @@ void G1RemSet::prepare_for_oops_into_collection_set_do() {
   _total_cards_scanned = 0;
 }
 
-
-// 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
-// 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.
-class UpdateRSetCardTableEntryIntoCSetClosure: public CardTableEntryClosure {
-  G1CollectedHeap* _g1;
-  CardTableModRefBS* _ct_bs;
-public:
-  UpdateRSetCardTableEntryIntoCSetClosure(G1CollectedHeap* g1,
-                                          CardTableModRefBS* bs):
-    _g1(g1), _ct_bs(bs)
-  { }
-
-  bool do_card_ptr(jbyte* card_ptr, uint worker_i) {
-    // Construct the region representing the card.
-    HeapWord* start = _ct_bs->addr_for(card_ptr);
-    // And find the region containing it.
-    HeapRegion* r = _g1->heap_region_containing(start);
-
-    // Scan oops in the card looking for references into the collection set
-    // Don't use addr_for(card_ptr + 1) which can ask for
-    // a card beyond the heap.  This is not safe without a perm
-    // gen.
-    HeapWord* end   = start + CardTableModRefBS::card_size_in_words;
-    MemRegion scanRegion(start, end);
-
-    UpdateRSetImmediate update_rs_cl(_g1->g1_rem_set());
-    FilterIntoCSClosure update_rs_cset_oop_cl(NULL, _g1, &update_rs_cl);
-    FilterOutOfRegionClosure filter_then_update_rs_cset_oop_cl(r, &update_rs_cset_oop_cl);
-
-    // We can pass false as the "filter_young" parameter here as:
-    // * we should be in a STW pause,
-    // * the DCQS to which this closure is applied is used to hold
-    //   references that point into the collection set from the prior
-    //   RSet updating,
-    // * the post-write barrier shouldn't be logging updates to young
-    //   regions (but there is a situation where this can happen - see
-    //   the comment in G1RemSet::refine_card() below -
-    //   that should not be applicable here), and
-    // * during actual RSet updating, the filtering of cards in young
-    //   regions in HeapRegion::oops_on_card_seq_iterate_careful is
-    //   employed.
-    // As a result, when this closure is applied to "refs into cset"
-    // DCQS, we shouldn't see any cards in young regions.
-    update_rs_cl.set_region(r);
-    HeapWord* stop_point =
-      r->oops_on_card_seq_iterate_careful(scanRegion,
-                                          &filter_then_update_rs_cset_oop_cl,
-                                          false /* filter_young */,
-                                          NULL  /* card_ptr */);
-
-    // Since this is performed in the event of an evacuation failure, we
-    // we shouldn't see a non-null stop point
-    assert(stop_point == NULL, "saw an unallocated region");
-    return true;
-  }
-};
-
 void G1RemSet::cleanup_after_oops_into_collection_set_do() {
   guarantee( _cards_scanned != NULL, "invariant" );
   _total_cards_scanned = 0;
@@ -465,25 +400,10 @@ void G1RemSet::cleanup_after_oops_into_collection_set_do() {
     double restore_remembered_set_start = os::elapsedTime();
 
     // Restore remembered sets for the regions pointing into the collection set.
-    if (G1DeferredRSUpdate) {
-      // If deferred RS updates are enabled then we just need to transfer
-      // the completed buffers from (a) the DirtyCardQueueSet used to hold
-      // cards that contain references that point into the collection set
-      // to (b) the DCQS used to hold the deferred RS updates
-      _g1->dirty_card_queue_set().merge_bufferlists(&into_cset_dcqs);
-    } else {
-
-      CardTableModRefBS* bs = (CardTableModRefBS*)_g1->barrier_set();
-      UpdateRSetCardTableEntryIntoCSetClosure update_rs_cset_immediate(_g1, bs);
-
-      int n_completed_buffers = 0;
-      while (into_cset_dcqs.apply_closure_to_completed_buffer(&update_rs_cset_immediate,
-                                                    0, 0, true)) {
-        n_completed_buffers++;
-      }
-      assert(n_completed_buffers == into_cset_n_buffers, "missed some buffers");
-    }
-
+    // We just need to transfer the completed buffers from the DirtyCardQueueSet
+    // used to hold cards that contain references that point into the collection set
+    // to the DCQS used to hold the deferred RS updates.
+    _g1->dirty_card_queue_set().merge_bufferlists(&into_cset_dcqs);
     _g1->g1_policy()->phase_times()->record_evac_fail_restore_remsets((os::elapsedTime() - restore_remembered_set_start) * 1000.0);
   }
 

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

@@ -193,18 +193,4 @@ public:
   bool apply_to_weak_ref_discovered_field() { return true; }
 };
 
-class UpdateRSetImmediate: public OopsInHeapRegionClosure {
-private:
-  G1RemSet* _g1_rem_set;
-
-  template <class T> void do_oop_work(T* p);
-public:
-  UpdateRSetImmediate(G1RemSet* rs) :
-    _g1_rem_set(rs) {}
-
-  virtual void do_oop(narrowOop* p) { do_oop_work(p); }
-  virtual void do_oop(      oop* p) { do_oop_work(p); }
-};
-
-
 #endif // SHARE_VM_GC_IMPLEMENTATION_G1_G1REMSET_HPP

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

@@ -79,13 +79,4 @@ inline void UpdateRSOopClosure::do_oop_work(T* p) {
   _rs->par_write_ref(_from, p, _worker_i);
 }
 
-template <class T>
-inline void UpdateRSetImmediate::do_oop_work(T* p) {
-  assert(_from->is_in_reserved(p), "paranoia");
-  T heap_oop = oopDesc::load_heap_oop(p);
-  if (!oopDesc::is_null(heap_oop) && !_from->is_survivor()) {
-    _g1_rem_set->par_write_ref(_from, p, 0);
-  }
-}
-
 #endif // SHARE_VM_GC_IMPLEMENTATION_G1_G1REMSET_INLINE_HPP

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

@@ -108,9 +108,6 @@
   develop(bool, G1RSBarrierRegionFilter, true,                              \
           "If true, generate region filtering code in RS barrier")          \
                                                                             \
-  develop(bool, G1DeferredRSUpdate, true,                                   \
-          "If true, use deferred RS updates")                               \
-                                                                            \
   develop(bool, G1RSLogCheckCardTable, false,                               \
           "If true, verify that no dirty cards remain after RS log "        \
           "processing.")                                                    \

test/gc/g1/TestDeferredRSUpdate.java

-/*
- * Copyright (c) 2014, 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
- * under the terms of the GNU General Public License version 2 only, as
- * published by the Free Software Foundation.
- *
- * This code is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
- * version 2 for more details (a copy is included in the LICENSE file that
- * accompanied this code).
- *
- * You should have received a copy of the GNU General Public License version
- * 2 along with this work; if not, write to the Free Software Foundation,
- * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
- * or visit www.oracle.com if you need additional information or have any
- * questions.
- */
-
-/*
- * @test TestDeferredRSUpdate
- * @bug 8040977 8052170
- * @summary Ensure that running with -XX:-G1DeferredRSUpdate does not crash the VM
- * @key gc
- * @library /testlibrary
- */
-
-import com.oracle.java.testlibrary.ProcessTools;
-import com.oracle.java.testlibrary.OutputAnalyzer;
-
-public class TestDeferredRSUpdate {
-  public static void main(String[] args) throws Exception {
-    GCTest.main(args);
-
-    ProcessBuilder pb = ProcessTools.createJavaProcessBuilder("-XX:+UseG1GC",
-                                                              "-Xmx10M",
-                                                              "-XX:+PrintGCDetails",
-                                                              // G1DeferredRSUpdate is a develop option, but we cannot limit execution of this test to only debug VMs.
-                                                              "-XX:+IgnoreUnrecognizedVMOptions",
-                                                              "-XX:-G1DeferredRSUpdate",
-                                                              GCTest.class.getName());
-
-    OutputAnalyzer output = new OutputAnalyzer(pb.start());
-    output.shouldHaveExitValue(0);
-  }
-
-  static class GCTest {
-    private static Object[] garbage = new Object[32];
-
-    public static void main(String [] args) {
-      System.out.println("Creating garbage");
-      // Create 128MB of garbage. This should result in at least one minor GC, with
-      // some objects copied to old gen. As references from old to young are installed,
-      // the crash due to the use before initialize occurs.
-      Object prev = null;
-      Object prevPrev = null;
-      for (int i = 0; i < 1024; i++) {
-        Object[] next = new Object[32 * 1024];
-        next[0] = prev;
-        next[1] = prevPrev;
-
-        Object[] cur = (Object[]) garbage[i % garbage.length];
-        if (cur != null) {
-          cur[0] = null;
-          cur[1] = null;
-        }
-        garbage[i % garbage.length] = next;
-
-        prevPrev = prev;
-        prev = next;
-      }
-      System.out.println("Done");
-    }
-  }
-}