RFR: JDK-8262068: Improve G1 Full GC by skipping compaction for regions with high survival ratio [v11]

[Thomas Schatzl]

On Thu, 25 Mar 2021 13:55:24 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Hamlin Li has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits:
>> 
>>  - Merge branch 'master' into g1-full-gc-optimization-00
>>  - reuse the "pin" mechanism (G1FullCollector._region_attr_table) to skip region compaction;
>>    deal with last-ditch full gc;
>>  - Merge branch 'master' into g1-full-gc-optimization-00
>>  - fix bot crash.
>>  - fix crash in G1CalculatePointersClosure::prepare_for_skipping_compaction when klass of dead objects is unloaded;
>>    other misc improvements.
>>  - reuse vm option MarkSweepDeadRatio; reuse G1RegionMarkStatsCache class; fix regression in Mark phase by inlining live words collection into mark_object()
>>  - JDK-8262068: Improve G1 Full GC by skipping compaction for regions with high survival ratio
>
> src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 43:
> 
>> 41: 
>> 42: bool G1FullGCPrepareTask::G1CalculatePointersClosure::do_heap_region(HeapRegion* hr) {
>> 43:   bool force_pinned = false;
> 
> Unless I'm missing something we could here do a call to a function called something like: `should_compact()` that checks the liveness of the heap region and if it should not be compacted it makes the region pinned() so that the check below will return true and thus avoid compacting it.
> 
> Doing this we will have to make sure the regions is "unpinned" at the end of the GC, and that could be handled by the `G1ResetPinnedClosure`. We might need to add a new `HeapRegionType` for this, something like `PinnedForGC`.

I think the current mechanism is fine, just "pin" it internally in that region attribute table. Do not really make the region "pinned" as suggested, that makes things just more complicated as you need to remember which regions are temporarily pinned.
This also adds another state into `HeapRegionType` for this special case that is already handled well by the region attribute table; the problem is that if we went that way, with region pinning (instead of GCLocker) we may also need to have all combinations of `PinnedForGC` with Eden/Survivor/Old etc.

Keeping the complexity here (and in that Full GC attribute table), we do not change "global" region state unnecessarily.

Still I think the code could be cleaned up by first determining that `should_compact` predicate.

Something like:

  
    if (!should_compact(hr)) { // probably it is better to avoid the negation here
      if (hr->is_humongous()) {
        ...
      } else if (hr->is_open_archive())) {
  
      } else {
        // otherwise compacted region, i.e.
        assert(hr->is_closed_archive() || liveness > threshold)
      }
    } else {
      prepare_for_compaction(hr);
    }

> src/hotspot/share/gc/g1/g1FullGCScope.cpp line 85:
> 
>> 83: }
>> 84: 
>> 85: size_t G1FullGCScope::hr_live_words_threshold() {
> 
> Not sure about the name, but can't come up with something a lot better, maybe `region_compaction_threshold()` and a comment explaining that if the amount of live data is greater than this value the region won't be compacted.

+1

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

PR: https://git.openjdk.java.net/jdk/pull/2760