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

[Thomas Schatzl]

On Mon, 17 Mar 2025 11:19:02 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi all,
> 
> Please review this change which aims to reduce spikes in mixed GCs, especially the last mixed-gc in a mixed cycle. Currently, G1 sorts regions identified for collection by reclaimable bytes, then prunes the list removing regions that with the lowest amount of reclaimable bytes. The pruned list is then split into collection groups which are later sorted on gc-efficiency. 
> 
> In the cachestress benchmark, we run into a case where some regions contain onlya  few live objects but having many incoming references from other regions.  These regions very expensive collect (low gc-efficiency).
> 
> This patch improves the pruning process by tracking incoming references to regions during marking. Instead of pruning based on reclaimable bytes alone, we estimate GC efficiency beforehand and prune regions with the worst GC efficiency.
> 
> This reduces the spikes in gc pause time as shown for cachestress benchmark in the image below. 
> 
> ![mixed-gc](https://github.com/user-attachments/assets/740fb51d-eb20-4946-bf90-4eef23afe2e4)
> 
> 
> Testing: Tier 1-3.

Looks good to me, thanks for removing the double-pruning from the initial protoype!

Did you ever try to get statistics about differences in marking length and changes to the cache hits in the mark stats cache? (Just curious)

src/hotspot/share/gc/g1/g1CollectionSetCandidates.cpp line 137:

> 135: }
> 136: 
> 137: int G1CollectionSetCandidateInfo::compare_gc_efficiency(G1CollectionSetCandidateInfo* ci1, G1CollectionSetCandidateInfo* ci2) {

Maybe make this name a bit more distinct than above `compare_gc_efficiency`, i.e. `compare_region_gc_efficiency` vs. `group_efficiency`.
Maybe however the different parameters are fine to distinguish them.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1291:

> 1289:         }
> 1290:       } else if (hr->is_old()) {
> 1291:         hr->note_end_of_marking(_cm->top_at_mark_start(hr), _cm->live_bytes(hr->hrm_index()), _cm->incoming_refs(hr->hrm_index()));

Potentially put `hr->hrm_index()` into a local so that the line is not that long any more; or just change the parameters to take a `HeapRegion*` for both - afaict they both are always called with `hr->hrm_index()`...

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp line 566:

> 564:   void set_live_bytes(uint region, size_t live_bytes) { _region_mark_stats[region]._live_words = live_bytes / HeapWordSize; }
> 565: 
> 566:   size_t incoming_refs(uint region) const { return _region_mark_stats[region]._incoming_refs; }

Suggestion:

  // Approximate number of incoming references found during marking.
  size_t incoming_refs(uint region) const { return _region_mark_stats[region]._incoming_refs; }

src/hotspot/share/gc/g1/g1HeapRegion.cpp line 346:

> 344:   G1Policy* p = G1CollectedHeap::heap()->policy();
> 345: 
> 346:   double merge_scan_time_ms = p->predict_merge_scan_time(_incoming_refs); // We use _incoming_refs as an estimate for remset cards

Suggestion:

  double merge_scan_time_ms = p->predict_merge_scan_time(_incoming_refs); // We use the number of incoming references as an estimate for remset cards.

src/hotspot/share/gc/g1/g1HeapRegion.hpp line 352:

> 350:   // GC Efficiency for collecting this region based on the time estimate in
> 351:   // total_based_on_incoming_refs_ms.
> 352:   double gc_efficiency();

Suggestion:

  // GC efficiency for collecting this region based on the time estimate in
  // total_based_on_incoming_refs_ms.
  double gc_efficiency();

src/hotspot/share/gc/g1/g1HeapRegion.hpp line 369:

> 367: 
> 368:   // Notify the region that concurrent marking has finished. Passes TAMS and the number of
> 369:   // bytes marked between bottom and TAMS.

Suggestion:

  // 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.

src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp line 42:

> 40: struct G1RegionMarkStats {
> 41:   size_t _live_words;
> 42:   size_t _incoming_refs;

The comment above needs update.

-------------

Changes requested by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24076#pullrequestreview-2694081010
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000803063
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000808520
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000810079
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000811646
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000812718
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000814177
PR Review Comment: https://git.openjdk.org/jdk/pull/24076#discussion_r2000815004