8274516: [REDO] JDK-8271880: Tighten condition for excluding regions …

…from collecting cards with cross-references

Reviewed-by: sjohanss, ayang

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

@@ -3272,6 +3272,7 @@ HeapRegion* G1CollectedHeap::new_gc_alloc_region(size_t word_size, G1HeapRegionA
       new_alloc_region->set_survivor();
       _survivor.add(new_alloc_region);
       _verifier->check_bitmaps("Survivor Region Allocation", new_alloc_region);
+      register_new_survivor_region_with_region_attr(new_alloc_region);
     } else {
       new_alloc_region->set_old();
       _verifier->check_bitmaps("Old Region Allocation", new_alloc_region);

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

@@ -605,6 +605,7 @@ class G1CollectedHeap : public CollectedHeap {
   void register_young_region_with_region_attr(HeapRegion* r) {
     _region_attr.set_in_young(r->hrm_index());
   }
+  inline void register_new_survivor_region_with_region_attr(HeapRegion* r);
   inline void register_region_with_region_attr(HeapRegion* r);
   inline void register_old_region_with_region_attr(HeapRegion* r);
   inline void register_optional_region_with_region_attr(HeapRegion* r);

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

@@ -184,6 +184,10 @@ void G1CollectedHeap::register_humongous_region_with_region_attr(uint index) {
   _region_attr.set_humongous(index, region_at(index)->rem_set()->is_tracked());
 }
 
+void G1CollectedHeap::register_new_survivor_region_with_region_attr(HeapRegion* r) {
+  _region_attr.set_new_survivor_region(r->hrm_index());
+}
+
 void G1CollectedHeap::register_region_with_region_attr(HeapRegion* r) {
   _region_attr.set_has_remset(r->hrm_index(), r->rem_set()->is_tracked());
 }

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

@@ -30,73 +30,30 @@
 #include "gc/g1/g1EvacFailureRegions.hpp"
 #include "gc/g1/g1HeapVerifier.hpp"
 #include "gc/g1/g1OopClosures.inline.hpp"
-#include "gc/g1/g1RedirtyCardsQueue.hpp"
 #include "gc/g1/heapRegion.hpp"
 #include "gc/g1/heapRegionRemSet.inline.hpp"
 #include "gc/shared/preservedMarks.inline.hpp"
 #include "oops/access.inline.hpp"
 #include "oops/compressedOops.inline.hpp"
 #include "oops/oop.inline.hpp"
 
-class UpdateLogBuffersDeferred : public BasicOopIterateClosure {
-private:
-  G1CollectedHeap* _g1h;
-  G1RedirtyCardsLocalQueueSet* _rdc_local_qset;
-  G1CardTable*    _ct;
-
-  // Remember the last enqueued card to avoid enqueuing the same card over and over;
-  // since we only ever handle a card once, this is sufficient.
-  size_t _last_enqueued_card;
-
-public:
-  UpdateLogBuffersDeferred(G1RedirtyCardsLocalQueueSet* rdc_local_qset) :
-    _g1h(G1CollectedHeap::heap()),
-    _rdc_local_qset(rdc_local_qset),
-    _ct(_g1h->card_table()),
-    _last_enqueued_card(SIZE_MAX) {}
-
-  virtual void do_oop(narrowOop* p) { do_oop_work(p); }
-  virtual void do_oop(      oop* p) { do_oop_work(p); }
-  template <class T> void do_oop_work(T* p) {
-    assert(_g1h->heap_region_containing(p)->is_in_reserved(p), "paranoia");
-    assert(!_g1h->heap_region_containing(p)->is_survivor(), "Unexpected evac failure in survivor region");
-
-    T const o = RawAccess<>::oop_load(p);
-    if (CompressedOops::is_null(o)) {
-      return;
-    }
-
-    if (HeapRegion::is_in_same_region(p, CompressedOops::decode(o))) {
-      return;
-    }
-    size_t card_index = _ct->index_for(p);
-    if (card_index != _last_enqueued_card) {
-      _rdc_local_qset->enqueue(_ct->byte_for_index(card_index));
-      _last_enqueued_card = card_index;
-    }
-  }
-};
-
 class RemoveSelfForwardPtrObjClosure: public ObjectClosure {
   G1CollectedHeap* _g1h;
   G1ConcurrentMark* _cm;
   HeapRegion* _hr;
   size_t _marked_bytes;
-  UpdateLogBuffersDeferred* _log_buffer_cl;
   bool _during_concurrent_start;
   uint _worker_id;
   HeapWord* _last_forwarded_object_end;
 
 public:
   RemoveSelfForwardPtrObjClosure(HeapRegion* hr,
-                                 UpdateLogBuffersDeferred* log_buffer_cl,
                                  bool during_concurrent_start,
                                  uint worker_id) :
     _g1h(G1CollectedHeap::heap()),
     _cm(_g1h->concurrent_mark()),
     _hr(hr),
     _marked_bytes(0),
-    _log_buffer_cl(log_buffer_cl),
     _during_concurrent_start(during_concurrent_start),
     _worker_id(worker_id),
     _last_forwarded_object_end(hr->bottom()) { }
@@ -141,20 +98,6 @@ class RemoveSelfForwardPtrObjClosure: public ObjectClosure {
       _marked_bytes += (obj_size * HeapWordSize);
       PreservedMarks::init_forwarded_mark(obj);
 
-      // While we were processing RSet buffers during the collection,
-      // we actually didn't scan any cards on the collection set,
-      // since we didn't want to update remembered sets with entries
-      // that point into the collection set, given that live objects
-      // from the collection set are about to move and such entries
-      // will be stale very soon.
-      // This change also dealt with a reliability issue which
-      // involved scanning a card in the collection set and coming
-      // across an array that was being chunked and looking malformed.
-      // The problem is that, if evacuation fails, we might have
-      // remembered set entries missing given that we skipped cards on
-      // the collection set. So, we'll recreate such entries now.
-      obj->oop_iterate(_log_buffer_cl);
-
       HeapWord* obj_end = obj_addr + obj_size;
       _last_forwarded_object_end = obj_end;
       _hr->alloc_block_in_bot(obj_addr, obj_end);
@@ -203,33 +146,22 @@ class RemoveSelfForwardPtrHRClosure: public HeapRegionClosure {
   G1CollectedHeap* _g1h;
   uint _worker_id;
 
-  G1RedirtyCardsLocalQueueSet _rdc_local_qset;
-  UpdateLogBuffersDeferred _log_buffer_cl;
-
   uint volatile* _num_failed_regions;
   G1EvacFailureRegions* _evac_failure_regions;
 
 public:
-  RemoveSelfForwardPtrHRClosure(G1RedirtyCardsQueueSet* rdcqs,
-                                uint worker_id,
+  RemoveSelfForwardPtrHRClosure(uint worker_id,
                                 uint volatile* num_failed_regions,
                                 G1EvacFailureRegions* evac_failure_regions) :
     _g1h(G1CollectedHeap::heap()),
     _worker_id(worker_id),
-    _rdc_local_qset(rdcqs),
-    _log_buffer_cl(&_rdc_local_qset),
     _num_failed_regions(num_failed_regions),
     _evac_failure_regions(evac_failure_regions) {
   }
 
-  ~RemoveSelfForwardPtrHRClosure() {
-    _rdc_local_qset.flush();
-  }
-
   size_t remove_self_forward_ptr_by_walking_hr(HeapRegion* hr,
                                                bool during_concurrent_start) {
     RemoveSelfForwardPtrObjClosure rspc(hr,
-                                        &_log_buffer_cl,
                                         during_concurrent_start,
                                         _worker_id);
     hr->object_iterate(&rspc);
@@ -268,17 +200,15 @@ class RemoveSelfForwardPtrHRClosure: public HeapRegionClosure {
   }
 };
 
-G1ParRemoveSelfForwardPtrsTask::G1ParRemoveSelfForwardPtrsTask(G1RedirtyCardsQueueSet* rdcqs,
-                                                               G1EvacFailureRegions* evac_failure_regions) :
+G1ParRemoveSelfForwardPtrsTask::G1ParRemoveSelfForwardPtrsTask(G1EvacFailureRegions* evac_failure_regions) :
   AbstractGangTask("G1 Remove Self-forwarding Pointers"),
   _g1h(G1CollectedHeap::heap()),
-  _rdcqs(rdcqs),
   _hrclaimer(_g1h->workers()->active_workers()),
   _evac_failure_regions(evac_failure_regions),
   _num_failed_regions(0) { }
 
 void G1ParRemoveSelfForwardPtrsTask::work(uint worker_id) {
-  RemoveSelfForwardPtrHRClosure rsfp_cl(_rdcqs, worker_id, &_num_failed_regions, _evac_failure_regions);
+  RemoveSelfForwardPtrHRClosure rsfp_cl(worker_id, &_num_failed_regions, _evac_failure_regions);
 
   // Iterate through all regions that failed evacuation during the entire collection.
   _evac_failure_regions->par_iterate(&rsfp_cl, &_hrclaimer, worker_id);

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

@@ -32,21 +32,19 @@
 
 class G1CollectedHeap;
 class G1EvacFailureRegions;
-class G1RedirtyCardsQueueSet;
 
 // Task to fixup self-forwarding pointers
 // installed as a result of an evacuation failure.
 class G1ParRemoveSelfForwardPtrsTask: public AbstractGangTask {
 protected:
   G1CollectedHeap* _g1h;
-  G1RedirtyCardsQueueSet* _rdcqs;
   HeapRegionClaimer _hrclaimer;
 
   G1EvacFailureRegions* _evac_failure_regions;
   uint volatile _num_failed_regions;
 
 public:
-  G1ParRemoveSelfForwardPtrsTask(G1RedirtyCardsQueueSet* rdcqs, G1EvacFailureRegions* evac_failure_regions);
+  G1ParRemoveSelfForwardPtrsTask(G1EvacFailureRegions* evac_failure_regions);
 
   void work(uint worker_id);
 

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

@@ -57,8 +57,9 @@ struct G1HeapRegionAttr {
   //
   // The other values are used for objects in regions requiring various special handling,
   // eager reclamation of humongous objects or optional regions.
-  static const region_type_t Optional     =  -3;    // The region is optional not in the current collection set.
-  static const region_type_t Humongous    =  -2;    // The region is a humongous candidate not in the current collection set.
+  static const region_type_t Optional     =  -4;    // The region is optional not in the current collection set.
+  static const region_type_t Humongous    =  -3;    // The region is a humongous candidate not in the current collection set.
+  static const region_type_t NewSurvivor  =  -2;    // The region is a new (ly allocated) survivor region.
   static const region_type_t NotInCSet    =  -1;    // The region is not in the collection set.
   static const region_type_t Young        =   0;    // The region is in the collection set and a young region.
   static const region_type_t Old          =   1;    // The region is in the collection set and an old region.
@@ -76,6 +77,7 @@ struct G1HeapRegionAttr {
     switch (type()) {
       case Optional: return "Optional";
       case Humongous: return "Humongous";
+      case NewSurvivor: return "NewSurvivor";
       case NotInCSet: return "NotInCSet";
       case Young: return "Young";
       case Old: return "Old";
@@ -85,6 +87,7 @@ struct G1HeapRegionAttr {
 
   bool needs_remset_update() const     { return _needs_remset_update != 0; }
 
+  void set_new_survivor()              { _type = NewSurvivor; }
   void set_old()                       { _type = Old; }
   void clear_humongous()               {
     assert(is_humongous() || !is_in_cset(), "must be");
@@ -96,6 +99,7 @@ struct G1HeapRegionAttr {
   bool is_in_cset() const              { return type() >= Young; }
 
   bool is_humongous() const            { return type() == Humongous; }
+  bool is_new_survivor() const         { return type() == NewSurvivor; }
   bool is_young() const                { return type() == Young; }
   bool is_old() const                  { return type() == Old; }
   bool is_optional() const             { return type() == Optional; }
@@ -128,6 +132,12 @@ class G1HeapRegionAttrBiasedMappedArray : public G1BiasedMappedArray<G1HeapRegio
     set_by_index(index, G1HeapRegionAttr(G1HeapRegionAttr::Optional, needs_remset_update));
   }
 
+  void set_new_survivor_region(uintptr_t index) {
+    assert(get_by_index(index).is_default(),
+           "Region attributes at index " INTPTR_FORMAT " should be default but is %s", index, get_by_index(index).get_type_str());
+    get_ref_by_index(index)->set_new_survivor();
+  }
+
   void set_humongous(uintptr_t index, bool needs_remset_update) {
     assert(get_by_index(index).is_default(),
            "Region attributes at index " INTPTR_FORMAT " should be default but is %s", index, get_by_index(index).get_type_str());

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

@@ -116,9 +116,9 @@ class G1SkipCardEnqueueSetter : public StackObj {
   G1ScanEvacuatedObjClosure* _closure;
 
 public:
-  G1SkipCardEnqueueSetter(G1ScanEvacuatedObjClosure* closure, bool new_value) : _closure(closure) {
+  G1SkipCardEnqueueSetter(G1ScanEvacuatedObjClosure* closure, bool skip_card_enqueue) : _closure(closure) {
     assert(_closure->_skip_card_enqueue == G1ScanEvacuatedObjClosure::Uninitialized, "Must not be set");
-    _closure->_skip_card_enqueue = new_value ? G1ScanEvacuatedObjClosure::True : G1ScanEvacuatedObjClosure::False;
+    _closure->_skip_card_enqueue = skip_card_enqueue ? G1ScanEvacuatedObjClosure::True : G1ScanEvacuatedObjClosure::False;
   }
 
   ~G1SkipCardEnqueueSetter() {

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

@@ -235,8 +235,8 @@ void G1ParScanThreadState::do_partial_array(PartialArrayScanTask task) {
     push_on_queue(ScannerTask(PartialArrayScanTask(from_obj)));
   }
 
-  HeapRegion* hr = _g1h->heap_region_containing(to_array);
-  G1SkipCardEnqueueSetter x(&_scanner, hr->is_young());
+  G1HeapRegionAttr dest_attr = _g1h->region_attr(to_array);
+  G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_new_survivor());
   // Process claimed task.  The length of to_array is not correct, but
   // fortunately the iteration ignores the length field and just relies
   // on start/end.
@@ -268,6 +268,11 @@ void G1ParScanThreadState::start_partial_objarray(G1HeapRegionAttr dest_attr,
     push_on_queue(ScannerTask(PartialArrayScanTask(from_obj)));
   }
 
+  // Skip the card enqueue iff the object (to_array) is in survivor region.
+  // However, HeapRegion::is_survivor() is too expensive here.
+  // Instead, we use dest_attr.is_young() because the two values are always
+  // equal: successfully allocated young regions must be survivor regions.
+  assert(dest_attr.is_young() == _g1h->heap_region_containing(to_array)->is_survivor(), "must be");
   G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young());
   // Process the initial chunk.  No need to process the type in the
   // klass, as it will already be handled by processing the built-in
@@ -519,6 +524,11 @@ oop G1ParScanThreadState::do_copy_to_survivor_space(G1HeapRegionAttr const regio
       _string_dedup_requests.add(old);
     }
 
+    // Skip the card enqueue iff the object (obj) is in survivor region.
+    // However, HeapRegion::is_survivor() is too expensive here.
+    // Instead, we use dest_attr.is_young() because the two values are always
+    // equal: successfully allocated young regions must be survivor regions.
+    assert(dest_attr.is_young() == _g1h->heap_region_containing(obj)->is_survivor(), "must be");
     G1SkipCardEnqueueSetter x(&_scanner, dest_attr.is_young());
     obj->oop_iterate_backwards(&_scanner, klass);
     return obj;
@@ -605,7 +615,14 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz
     _preserved_marks->push_if_necessary(old, m);
     _evacuation_failed_info.register_copy_failure(word_sz);
 
-    G1SkipCardEnqueueSetter x(&_scanner, r->is_young());
+    // For iterating objects that failed evacuation currently we can reuse the
+    // existing closure to scan evacuated objects because:
+    // - for objects referring into the collection set we do not need to gather
+    // cards at this time. The regions they are in will be unconditionally turned
+    // to old regions without remembered sets.
+    // - since we are iterating from a collection set region (i.e. never a Survivor
+    // region), we always need to gather cards for this case.
+    G1SkipCardEnqueueSetter x(&_scanner, false /* skip_card_enqueue */);
     old->oop_iterate_backwards(&_scanner);
 
     return old;

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

@@ -134,7 +134,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
 
   // Apply the post barrier to the given reference field. Enqueues the card of p
   // if the barrier does not filter out the reference for some reason (e.g.
-  // p and q are in the same region, p is in survivor)
+  // p and q are in the same region, p is in survivor, p is in collection set)
   // To be called during GC if nothing particular about p and obj are known.
   template <class T> void write_ref_field_post(T* p, oop obj);
 

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

@@ -97,19 +97,34 @@ G1OopStarChunkedList* G1ParScanThreadState::oops_into_optional_region(const Heap
 }
 
 template <class T> void G1ParScanThreadState::write_ref_field_post(T* p, oop obj) {
-  assert(obj != NULL, "Must be");
+  assert(obj != nullptr, "Must be");
   if (HeapRegion::is_in_same_region(p, obj)) {
     return;
   }
-  HeapRegion* from = _g1h->heap_region_containing(p);
-  if (!from->is_young()) {
-    enqueue_card_if_tracked(_g1h->region_attr(obj), p, obj);
+  G1HeapRegionAttr from_attr = _g1h->region_attr(p);
+  // If this is a reference from (current) survivor regions, we do not need
+  // to track references from it.
+  if (from_attr.is_new_survivor()) {
+    return;
+  }
+  G1HeapRegionAttr dest_attr = _g1h->region_attr(obj);
+  // References to the current collection set are references to objects that failed
+  // evacuation. Currently these regions are always relabelled as old without
+  // remembered sets, so skip them.
+  assert(dest_attr.is_in_cset() == (obj->forwardee() == obj),
+         "Only evac-failed objects must be in the collection set here but " PTR_FORMAT " is not", p2i(obj));
+  if (dest_attr.is_in_cset()) {
+    return;
   }
+  enqueue_card_if_tracked(dest_attr, p, obj);
 }
 
 template <class T> void G1ParScanThreadState::enqueue_card_if_tracked(G1HeapRegionAttr region_attr, T* p, oop o) {
   assert(!HeapRegion::is_in_same_region(p, o), "Should have filtered out cross-region references already.");
-  assert(!_g1h->heap_region_containing(p)->is_young(), "Should have filtered out from-young references already.");
+  assert(!_g1h->heap_region_containing(p)->is_survivor(), "Should have filtered out from-newly allocated survivor references already.");
+  // We relabel all regions that failed evacuation as old gen without remembered,
+  // and so pre-filter them out in the caller.
+  assert(!_g1h->heap_region_containing(o)->in_collection_set(), "Should not try to enqueue reference into collection set region");
 
 #ifdef ASSERT
   HeapRegion* const hr_obj = _g1h->heap_region_containing(o);