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

[Stefan Johansson]

On Thu, 25 Mar 2021 12:28:58 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Summary
>> -----------
>> 
>> Improve G1 Full GC by skip compaction for regions with high survival ratio.
>> 
>> Backgroud
>> -----------
>> 
>> There are 4 steps in full gc of G1 GC.
>> - mark live objects
>> - prepare forwardee
>> - adjust pointers
>> - compact
>> 
>> When full gc occurs, there may be very high percentage of live bytes in some regions. For these regions, it's not efficient to compact them and better to skip them, as there are little space to save but many objects to copy.
>> 
>> Description
>> -----------
>> 
>> We enhance the full gc implementation for the above situation through following steps:
>> - accumulate live bytes of every hr in mark phase; (already done by JDK-8263495)
>> - skip adding regions with high survial ratio, and set the region with high survival ratio as pinned in _region_attr_table during prepare phase;
>> - nothing special is done in adjust phase, regions with high survial ratio are skipped because of pin setting in the above step;
>> - nothing special is done in compact phase, regions with high survival ratio are skipped because these regions are skipped when adding regions to compaction set in the prepare phase;
>> 
>> VM options related
>> -----------
>> 
>> - MarkSweepDeadRatio: we reuse this exising vm option to indicate the high survial ratio threhold (100-MarkSweepDeadRatio) in G1.
>>      - default value of MarkSweepDeadRatio: 5 
>> 
>> Test
>> -----------
>> 
>> - specjbb2015: no regression
>> - dacapo: (Attachment is the dacapo h2 full gc pause.)
>>      - 95% of full gc pauses: 10%-19% improvement. 
>>      - 5% of full gc pauses: 1.2% improvement.
>>      - 0.1% of full gc pauses: -6.16% improvement.
>> 
>> $ java -Xmx1g -Xms1g -XX:ParallelGCThreads=4 -Xlog:gc*=info:file=gc.log -jar dacapo-9.12-bach.jar --iterations 5 --size huge --no-pre-iteration-gc h2
>
> 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

This is starting to look really nice. A few suggestions below. I did not comment on all places where I think we should change *maximal* to *maximum*, and we might want to hear from others what they think about this as well. 

The only big thing is how to mark the region as "pinned during the GC", I hope my suggestion will work, I think it would make this feature fit even better.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1163:

> 1161: 
> 1162:   if (do_gc) {
> 1163:     bool do_maximal_compaction = clear_all_soft_refs;

Please add a comment here to explain that when clear_all_soft_refs is set we want to do a maximum compaction not leaving any dead wood around.

I also think *maximum* is more appropriate than *maximal*.

src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 507:

> 505:   bool do_full_collection(bool explicit_gc,
> 506:                           bool clear_all_soft_refs,
> 507:                           bool do_maximal_compaction = false);

I would prefer if we don't have a default value here but instead pass `false` explicitly on the needed places. Shouldn't be that many.

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

src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 78:

> 76:       log_debug(gc, phases)("Phase 2: skip compaction region index: %u, live words: " SIZE_FORMAT,
> 77:                             hr->hrm_index(), live_words);
> 78:     }

Doing the above, all this can go back to just being:
`prepare_for_compaction(hr);`

And you can add the logging in the pinned section above, something like:
else if(hr->is_pinned_for_gc()) {
  assert(MarkSweepDeadRatio > 0,
         "it should not trigger skipping compaction, when MarkSweepDeadRatio == 0");
  log_debug(gc, phases)("Phase 2: skip compaction region index: %u, live words: " SIZE_FORMAT,
                        hr->hrm_index(), live_words);
}

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.

src/hotspot/share/gc/shared/gc_globals.hpp line 678:

> 676:           "compaction of heap regions, i.e. if a heap region has dead "     \
> 677:           "bytes less than this value, then this region will not be "       \
> 678:           "compacted during G1 full GC.")                                   \

I would prefer garbage rather than dead bytes. What do you think about:
Suggestion:

          "G1 full gc treats this as an allowed garbage threshold to skip " \
          "compaction of heap regions, i.e. if a heap region has less "     \
          "garbage than this value, then the region will not be compacted " \
          "during G1 full GC.")                                             \

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

Changes requested by sjohanss (Reviewer).

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