8351405: G1: Collection set early pruning causes suboptimal region selection

Reviewed-by: ayang, tschatzl

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

@@ -134,7 +134,7 @@ int G1CSetCandidateGroup::compare_gc_efficiency(G1CSetCandidateGroup** gr1, G1CS
   }
 }
 
-int G1CSetCandidateGroup::compare_reclaimble_bytes(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2) {
+int G1CollectionSetCandidateInfo::compare_region_gc_efficiency(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2) {
   // Make sure that null entries are moved to the end.
   if (ci1->_r == nullptr) {
     if (ci2->_r == nullptr) {
@@ -146,12 +146,13 @@ int G1CSetCandidateGroup::compare_reclaimble_bytes(G1CollectionSetCandidateInfo*
     return -1;
   }
 
-  size_t reclaimable1 = ci1->_r->reclaimable_bytes();
-  size_t reclaimable2 = ci2->_r->reclaimable_bytes();
+  G1Policy* p = G1CollectedHeap::heap()->policy();
+  double gc_efficiency1 = p->predict_gc_efficiency(ci1->_r);
+  double gc_efficiency2 = p->predict_gc_efficiency(ci2->_r);
 
-  if (reclaimable1 > reclaimable2) {
+  if (gc_efficiency1 > gc_efficiency2) {
     return -1;
-  } else if (reclaimable1 < reclaimable2) {
+  } else if (gc_efficiency1 < gc_efficiency2) {
     return 1;
   } else {
     return 0;

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

@@ -48,6 +48,8 @@ struct G1CollectionSetCandidateInfo {
     ++_num_unreclaimed;
     return _num_unreclaimed < G1NumCollectionsKeepPinned;
   }
+
+  static int compare_region_gc_efficiency(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2);
 };
 
 using G1CSetCandidateGroupIterator = GrowableArrayIterator<G1CollectionSetCandidateInfo>;
@@ -106,8 +108,6 @@ public:
   // up at the end of the list.
   static int compare_gc_efficiency(G1CSetCandidateGroup** gr1, G1CSetCandidateGroup** gr2);
 
-  static int compare_reclaimble_bytes(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2);
-
   double gc_efficiency() const { return _gc_efficiency; }
 
   G1HeapRegion* region_at(uint i) const { return _candidates.at(i)._r; }

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

@@ -96,14 +96,14 @@ class G1BuildCandidateRegionsTask : public WorkerTask {
       _data[idx] = CandidateInfo(hr);
     }
 
-    void sort_by_reclaimable_bytes() {
+    void sort_by_gc_efficiency() {
       if (_cur_claim_idx == 0) {
         return;
       }
       for (uint i = _cur_claim_idx; i < _max_size; i++) {
         assert(_data[i]._r == nullptr, "must be");
       }
-      qsort(_data, _cur_claim_idx, sizeof(_data[0]), (_sort_Fn)G1CSetCandidateGroup::compare_reclaimble_bytes);
+      qsort(_data, _cur_claim_idx, sizeof(_data[0]), (_sort_Fn)G1CollectionSetCandidateInfo::compare_region_gc_efficiency);
       for (uint i = _cur_claim_idx; i < _max_size; i++) {
         assert(_data[i]._r == nullptr, "must be");
       }
@@ -247,7 +247,7 @@ public:
   }
 
   void sort_and_prune_into(G1CollectionSetCandidates* candidates) {
-    _result.sort_by_reclaimable_bytes();
+    _result.sort_by_gc_efficiency();
     prune(_result.array());
     candidates->set_candidates_from_marking(_result.array(),
                                             _num_regions_added);

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

@@ -1288,7 +1288,8 @@ class G1UpdateRegionLivenessAndSelectForRebuildTask : public WorkerTask {
           reclaim_empty_humongous_region(hr);
         }
       } else if (hr->is_old()) {
-        hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(hr->hrm_index()));
+        uint region_idx = hr->hrm_index();
+        hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(region_idx), _cm->incoming_refs(region_idx));
 
         if (hr->live_bytes() != 0) {
           if (tracker->update_old_before_rebuild(hr)) {

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

@@ -562,6 +562,8 @@ public:
   size_t live_bytes(uint region) const { return _region_mark_stats[region]._live_words * HeapWordSize; }
   // Set live bytes for concurrent marking.
   void set_live_bytes(uint region, size_t live_bytes) { _region_mark_stats[region]._live_words = live_bytes / HeapWordSize; }
+  // Approximate number of incoming references found during marking.
+  size_t incoming_refs(uint region) const { return _region_mark_stats[region]._incoming_refs; }
 
   // Update the TAMS for the given region to the current top.
   inline void update_top_at_mark_start(G1HeapRegion* r);
@@ -952,6 +954,8 @@ public:
 
   inline void update_liveness(oop const obj, size_t const obj_size);
 
+  inline void inc_incoming_refs(oop const obj);
+
   // Clear (without flushing) the mark cache entry for the given region.
   void clear_mark_stats_cache(uint region_idx);
   // Evict the whole statistics cache into the global statistics. Returns the

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

@@ -228,6 +228,10 @@ inline void G1CMTask::update_liveness(oop const obj, const size_t obj_size) {
   _mark_stats_cache.add_live_words(_g1h->addr_to_region(obj), obj_size);
 }
 
+inline void G1CMTask::inc_incoming_refs(oop const obj) {
+  _mark_stats_cache.inc_incoming_refs(_g1h->addr_to_region(obj));
+}
+
 inline void G1ConcurrentMark::add_to_liveness(uint worker_id, oop const obj, size_t size) {
   task(worker_id)->update_liveness(obj, size);
 }
@@ -288,6 +292,10 @@ inline bool G1CMTask::deal_with_reference(T* p) {
   if (obj == nullptr) {
     return false;
   }
+
+  if (!G1HeapRegion::is_in_same_region(p, obj)) {
+    inc_incoming_refs(obj);
+  }
   return make_reference_grey(obj);
 }
 

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

@@ -128,6 +128,7 @@ void G1HeapRegion::hr_clear(bool clear_space) {
 
   _parsable_bottom = bottom();
   _garbage_bytes = 0;
+  _incoming_refs = 0;
 
   if (clear_space) clear(SpaceDecorator::Mangle);
 }
@@ -239,6 +240,7 @@ G1HeapRegion::G1HeapRegion(uint hrm_index,
 #endif
   _parsable_bottom(nullptr),
   _garbage_bytes(0),
+  _incoming_refs(0),
   _young_index_in_cset(-1),
   _surv_rate_group(nullptr),
   _age_index(G1SurvRateGroup::InvalidAgeIndex),
@@ -278,6 +280,7 @@ void G1HeapRegion::report_region_type_change(G1HeapRegionTraceType::Type to) {
   assert(parsable_bottom_acquire() == bottom(), "must be");
 
   _garbage_bytes = 0;
+  _incoming_refs = 0;
 }
 
 void G1HeapRegion::note_self_forward_chunk_done(size_t garbage_bytes) {

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

@@ -237,6 +237,10 @@ private:
   // Amount of dead data in the region.
   size_t _garbage_bytes;
 
+  // Approximate number of references to this regions at the end of concurrent
+  // marking. We we do not mark through all objects, so this is an estimate.
+  size_t _incoming_refs;
+
   // Data for young region survivor prediction.
   uint  _young_index_in_cset;
   G1SurvRateGroup* _surv_rate_group;
@@ -339,6 +343,8 @@ public:
     return capacity() - known_live_bytes;
   }
 
+  size_t incoming_refs() { return _incoming_refs; }
+
   inline bool is_collection_set_candidate() const;
 
   // Retrieve parsable bottom; since it may be modified concurrently, outside a
@@ -351,9 +357,9 @@ public:
   // that the collector is about to start or has finished (concurrently)
   // marking the heap.
 
-  // Notify the region that concurrent marking has finished. Passes TAMS and the number of
-  // bytes marked between bottom and TAMS.
-  inline void note_end_of_marking(HeapWord* top_at_mark_start, size_t marked_bytes);
+  // Notify the region that concurrent marking has finished. Passes TAMS, the number of
+  // bytes marked between bottom and TAMS, and the estimate for incoming references.
+  inline void note_end_of_marking(HeapWord* top_at_mark_start, size_t marked_bytes, size_t incoming_refs);
 
   // Notify the region that scrubbing has completed.
   inline void note_end_of_scrubbing();

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

@@ -156,6 +156,8 @@ inline void G1HeapRegion::reset_after_full_gc_common() {
 
   _garbage_bytes = 0;
 
+  _incoming_refs = 0;
+
   // Clear unused heap memory in debug builds.
   if (ZapUnusedHeapArea) {
     mangle_unused_area();
@@ -263,11 +265,12 @@ inline void G1HeapRegion::reset_parsable_bottom() {
   Atomic::release_store(&_parsable_bottom, bottom());
 }
 
-inline void G1HeapRegion::note_end_of_marking(HeapWord* top_at_mark_start, size_t marked_bytes) {
+inline void G1HeapRegion::note_end_of_marking(HeapWord* top_at_mark_start, size_t marked_bytes, size_t incoming_refs) {
   assert_at_safepoint();
 
   if (top_at_mark_start != bottom()) {
     _garbage_bytes = byte_size(bottom(), top_at_mark_start) - marked_bytes;
+    _incoming_refs = incoming_refs;
   }
 
   if (needs_scrubbing()) {

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

@@ -1107,6 +1107,15 @@ size_t G1Policy::predict_bytes_to_copy(G1HeapRegion* hr) const {
   return bytes_to_copy;
 }
 
+double G1Policy::predict_gc_efficiency(G1HeapRegion* hr) {
+
+  double total_based_on_incoming_refs_ms = predict_merge_scan_time(hr->incoming_refs()) + // We use the number of incoming references as an estimate for remset cards.
+                                           predict_non_young_other_time_ms(1) +
+                                           predict_region_copy_time_ms(hr, false /* for_young_only_phase */);
+
+  return hr->reclaimable_bytes() / total_based_on_incoming_refs_ms;
+}
+
 double G1Policy::predict_young_region_other_time_ms(uint count) const {
   return _analytics->predict_young_other_time_ms(count);
 }

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

@@ -243,6 +243,10 @@ public:
   size_t predict_bytes_to_copy(G1HeapRegion* hr) const;
   size_t pending_cards_at_gc_start() const { return _pending_cards_at_gc_start; }
 
+  // GC efficiency for collecting the region based on the time estimate for
+  // merging and scanning incoming references.
+  double predict_gc_efficiency(G1HeapRegion* hr);
+
   // The minimum number of retained regions we will add to the CSet during a young GC.
   uint min_retained_old_cset_length() const;
   // Calculate the minimum number of old regions we'll add to the CSet

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

@@ -33,20 +33,26 @@
 
 // Per-Region statistics gathered during marking.
 //
-// This includes
+// These include:
 // * the number of live words gathered during marking for the area from bottom
-// to tams. This is an exact measure.
-// The code corrects later for the live data between tams and top.
+//   to tams. This is an exact measure. The code corrects later for the live data
+//   between tams and top.
+// * the number of incoming references found during marking. This is an approximate
+//   value because we do not mark through all objects.
 struct G1RegionMarkStats {
   size_t _live_words;
+  size_t _incoming_refs;
 
   // Clear all members.
   void clear() {
     _live_words = 0;
+    _incoming_refs = 0;
   }
-  // Clear all members after a marking overflow. Nothing to do as the live words
-  // are updated by the atomic mark. We do not remark objects after overflow.
+  // Clear all members after a marking overflow. Only needs to clear the number of
+  // incoming references as all objects will be rescanned, while the live words are
+  // gathered whenever a thread can mark an object, which is synchronized.
   void clear_during_overflow() {
+    _incoming_refs = 0;
   }
 };
 
@@ -107,6 +113,11 @@ public:
     cur->_stats._live_words += live_words;
   }
 
+  void inc_incoming_refs(uint region_idx) {
+    G1RegionMarkStatsCacheEntry* const cur = find_for_add(region_idx);
+    cur->_stats._incoming_refs++;
+  }
+
   void reset(uint region_idx) {
     uint const cache_idx = hash(region_idx);
     G1RegionMarkStatsCacheEntry* cur = &_cache[cache_idx];

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

@@ -49,6 +49,11 @@ inline void G1RegionMarkStatsCache::evict(uint idx) {
   if (cur->_stats._live_words != 0) {
     Atomic::add(&_target[cur->_region_idx]._live_words, cur->_stats._live_words);
   }
+
+  if (cur->_stats._incoming_refs != 0) {
+    Atomic::add(&_target[cur->_region_idx]._incoming_refs, cur->_stats._incoming_refs);
+  }
+
   cur->clear();
 }