8267073: Race between Card Redirtying and Freeing Collection Set regions results in missing remembered set entries with G1

Reviewed-by: kbarrett, sjohanss

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -1477,6 +1477,7 @@ G1CollectedHeap::G1CollectedHeap() :
   _cr(NULL),
   _task_queues(NULL),
   _num_regions_failed_evacuation(0),
+  _regions_failed_evacuation(NULL),
   _evacuation_failed_info_array(NULL),
   _preserved_marks_set(true /* in_c_heap */),
 #ifndef PRODUCT
@@ -1769,6 +1770,8 @@ jint G1CollectedHeap::initialize() {
 
   _collection_set.initialize(max_reserved_regions());
 
+  _regions_failed_evacuation = NEW_C_HEAP_ARRAY(volatile bool, max_regions(), mtGC);
+
   G1InitLogger::print();
 
   return JNI_OK;
@@ -3467,6 +3470,8 @@ void G1CollectedHeap::pre_evacuate_collection_set(G1EvacuationInfo& evacuation_i
   _expand_heap_after_alloc_failure = true;
   Atomic::store(&_num_regions_failed_evacuation, 0u);
 
+  memset((void*)_regions_failed_evacuation, false, sizeof(bool) * max_regions());
+
   // Disable the hot card cache.
   _hot_card_cache->reset_hot_cache_claimed_index();
   _hot_card_cache->set_use_cache(false);

src/hotspot/share/gc/g1/g1CollectedHeap.hpp

@@ -867,6 +867,8 @@ public:
 
   // Number of regions evacuation failed in the current collection.
   volatile uint _num_regions_failed_evacuation;
+  // Records for every region on the heap whether evacuation failed for it.
+  volatile bool* _regions_failed_evacuation;
 
   EvacuationFailedInfo* _evacuation_failed_info_array;
 
@@ -1145,10 +1147,15 @@ public:
 
   // True iff an evacuation has failed in the most-recent collection.
   inline bool evacuation_failed() const;
+  // True iff the given region encountered an evacuation failure in the most-recent
+  // collection.
+  inline bool evacuation_failed(uint region_idx) const;
+
   inline uint num_regions_failed_evacuation() const;
-  // Notify that the garbage collection encountered an evacuation failure in a
-  // region. Should only be called once per region.
-  inline void notify_region_failed_evacuation();
+  // Notify that the garbage collection encountered an evacuation failure in the
+  // given region. Returns whether this has been the first occurrence of an evacuation
+  // failure in that region.
+  inline bool notify_region_failed_evacuation(uint const region_idx);
 
   void remove_from_old_gen_sets(const uint old_regions_removed,
                                 const uint archive_regions_removed,

src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp

@@ -194,12 +194,25 @@ bool G1CollectedHeap::evacuation_failed() const {
   return num_regions_failed_evacuation() > 0;
 }
 
+bool G1CollectedHeap::evacuation_failed(uint region_idx) const {
+  assert(region_idx < max_regions(), "Invalid region index %u", region_idx);
+
+  return Atomic::load(&_regions_failed_evacuation[region_idx]);
+}
+
 uint G1CollectedHeap::num_regions_failed_evacuation() const {
   return Atomic::load(&_num_regions_failed_evacuation);
 }
 
-void G1CollectedHeap::notify_region_failed_evacuation() {
-  Atomic::inc(&_num_regions_failed_evacuation, memory_order_relaxed);
+bool G1CollectedHeap::notify_region_failed_evacuation(uint const region_idx) {
+  assert(region_idx < max_regions(), "Invalid region index %u", region_idx);
+
+  volatile bool* region_failed_addr = &_regions_failed_evacuation[region_idx];
+  bool result = !Atomic::load(region_failed_addr) && !Atomic::cmpxchg(region_failed_addr, false, true, memory_order_relaxed);
+  if (result) {
+    Atomic::inc(&_num_regions_failed_evacuation, memory_order_relaxed);
+  }
+  return result;
 }
 
 #ifndef PRODUCT

src/hotspot/share/gc/g1/g1EvacFailure.cpp

@@ -237,7 +237,7 @@ public:
     assert(!hr->is_pinned(), "Unexpected pinned region at index %u", hr->hrm_index());
     assert(hr->in_collection_set(), "bad CS");
 
-    if (hr->evacuation_failed()) {
+    if (_g1h->evacuation_failed(hr->hrm_index())) {
       hr->clear_index_in_opt_cset();
 
       bool during_concurrent_start = _g1h->collector_state()->in_concurrent_start_gc();

src/hotspot/share/gc/g1/g1ParScanThreadState.cpp

@@ -605,8 +605,7 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m) {
     // Forward-to-self succeeded. We are the "owner" of the object.
     HeapRegion* r = _g1h->heap_region_containing(old);
 
-    if (r->set_evacuation_failed()) {
-      _g1h->notify_region_failed_evacuation();
+    if (_g1h->notify_region_failed_evacuation(r->hrm_index())) {
       _g1h->hr_printer()->evac_failure(r);
     }
 

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp

@@ -265,9 +265,9 @@ class RedirtyLoggedCardTableEntryClosure : public G1CardTableEntryClosure {
   }
 
   bool will_become_free(HeapRegion* hr) const {
-    // A region will be freed by free_collection_set if the region is in the
+    // A region will be freed by during the FreeCollectionSet phase if the region is in the
     // collection set and has not had an evacuation failure.
-    return _g1h->is_in_cset(hr) && !hr->evacuation_failed();
+    return _g1h->is_in_cset(hr) && !_g1h->evacuation_failed(hr->hrm_index());
   }
 
  public:
@@ -379,8 +379,8 @@ public:
   }
 
   void account_evacuated_region(HeapRegion* r) {
-      size_t used = r->used();
-      assert(used > 0, "region %u %s zero used", r->hrm_index(), r->get_short_type_str());
+    size_t used = r->used();
+    assert(used > 0, "region %u %s zero used", r->hrm_index(), r->get_short_type_str());
     _before_used_bytes += used;
     _regions_freed += 1;
   }
@@ -431,7 +431,7 @@ class FreeCSetClosure : public HeapRegionClosure {
   Tickspan         _non_young_time;
   FreeCSetStats*   _stats;
 
-  void assert_in_cset(HeapRegion* r) {
+  void assert_tracks_surviving_words(HeapRegion* r) {
     assert(r->young_index_in_cset() != 0 &&
            (uint)r->young_index_in_cset() <= _g1h->collection_set()->young_region_length(),
            "Young index %u is wrong for region %u of type %s with %u young regions",
@@ -486,15 +486,14 @@ public:
     JFREventForRegion event(r, _worker_id);
     TimerForRegion timer(timer_for_region(r));
 
-    _g1h->clear_region_attr(r);
     stats()->account_rs_length(r);
 
     if (r->is_young()) {
-      assert_in_cset(r);
+      assert_tracks_surviving_words(r);
       r->record_surv_words_in_group(_surviving_young_words[r->young_index_in_cset()]);
     }
 
-    if (r->evacuation_failed()) {
+    if (_g1h->evacuation_failed(r->hrm_index())) {
       handle_failed_region(r);
     } else {
       handle_evacuated_region(r);

src/hotspot/share/gc/g1/heapRegion.cpp

@@ -104,7 +104,6 @@ void HeapRegion::setup_heap_region_size(size_t max_heap_size) {
 void HeapRegion::handle_evacuation_failure() {
   uninstall_surv_rate_group();
   clear_young_index_in_cset();
-  reset_evacuation_failed();
   set_old();
   _next_marked_bytes = 0;
 }
@@ -118,8 +117,6 @@ void HeapRegion::unlink_from_list() {
 void HeapRegion::hr_clear(bool clear_space) {
   assert(_humongous_start_region == NULL,
          "we should have already filtered out humongous regions");
-  assert(!in_collection_set(),
-         "Should not clear heap region %u in the collection set", hrm_index());
 
   clear_young_index_in_cset();
   clear_index_in_opt_cset();
@@ -134,7 +131,6 @@ void HeapRegion::hr_clear(bool clear_space) {
   init_top_at_mark_start();
   if (clear_space) clear(SpaceDecorator::Mangle);
 
-  Atomic::store(&_evacuation_failed, false);
   _gc_efficiency = -1.0;
 }
 
@@ -242,7 +238,6 @@ HeapRegion::HeapRegion(uint hrm_index,
   _hrm_index(hrm_index),
   _type(),
   _humongous_start_region(NULL),
-  _evacuation_failed(false),
   _index_in_opt_cset(InvalidCSetIndex),
   _next(NULL), _prev(NULL),
 #ifdef ASSERT

src/hotspot/share/gc/g1/heapRegion.hpp

@@ -210,9 +210,6 @@ private:
   // For a humongous region, region in which it starts.
   HeapRegion* _humongous_start_region;
 
-  // True iff an attempt to evacuate an object in the region failed.
-  volatile bool _evacuation_failed;
-
   static const uint InvalidCSetIndex = UINT_MAX;
 
   // The index in the optional regions array, if this region
@@ -498,15 +495,6 @@ public:
   // Clear the card table corresponding to this region.
   void clear_cardtable();
 
-  // Returns the "evacuation_failed" property of the region.
-  inline bool evacuation_failed() const;
-
-  // Sets the "evacuation_failed" property of the region, returning true if this
-  // has been the first call, false otherwise.
-  inline bool set_evacuation_failed();
-
-  inline void reset_evacuation_failed();
-
   // Notify the region that we are about to start processing
   // self-forwarded objects during evac failure handling.
   void note_self_forwarding_removal_start(bool during_concurrent_start,

src/hotspot/share/gc/g1/heapRegion.inline.hpp

@@ -452,16 +452,4 @@ inline void HeapRegion::record_surv_words_in_group(size_t words_survived) {
   _surv_rate_group->record_surviving_words(age_in_group, words_survived);
 }
 
-inline bool HeapRegion::evacuation_failed() const {
-  return Atomic::load(&_evacuation_failed);
-}
-
-inline bool HeapRegion::set_evacuation_failed() {
-  return !Atomic::load(&_evacuation_failed) && !Atomic::cmpxchg(&_evacuation_failed, false, true, memory_order_relaxed);
-}
-
-inline void HeapRegion::reset_evacuation_failed() {
-  Atomic::store(&_evacuation_failed, false);
-}
-
 #endif // SHARE_GC_G1_HEAPREGION_INLINE_HPP