RFR: 8256265: G1: Improve parallelism in regions that failed evacuation [v11]

[Albert Mingkun Yang]

On Wed, 2 Mar 2022 14:28:51 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Currently G1 assigns a thread per failed evacuated region. This can in effect serialize the whole process as often (particularly with region pinning) there is only one region to fix up.
>> 
>> This patch tries to improve parallelism when walking over the regions in chunks
>> 
>> Latest implementation scans regions in chunks to bring parallelism, it's based on JDK-8278917 which changes to uses prev bitmap to mark evacuation failure objs.
>> 
>> Here's the summary of performance data based on latest implementation, basically, it brings better and stable performance than baseline at "Post Evacuate Cleanup 1/remove self forwardee" phase. (Although some regression is spotted when calculate the results in geomean, becuase one pause time from baseline is far too small than others.)
>> 
>> The performance benefit trend is:
>>  - pause time (Post Evacuate Cleanup 1) is decreased from 76.79% to 2.28% for average time, from 71.61% to 3.04% for geomean, when G1EvacuationFailureALotCSetPercent is changed from 2 to 90 (-XX:ParallelGCThreads=8)
>>  - pause time (Post Evacuate Cleanup 1) is decreased from 63.84% to 15.16% for average time, from 55.41% to 12.45% for geomean, when G1EvacuationFailureALotCSetPercent is changed from 2 to 90 (-XX:ParallelGCThreads=<default=123>)
>> ( Other common Evacuation Failure configurations are:
>> -XX:+G1EvacuationFailureALot -XX:G1EvacuationFailureALotInterval=0 -XX:G1EvacuationFailureALotCount=0 )
>> 
>> For more detailed performance data, please check the related bug.
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   move 'chunks per region' log

Some minor comments/suggestions.

src/hotspot/share/gc/g1/g1EvacFailure.cpp line 163:

> 161:     RegionMarkedWordsCache():
> 162:       _g1h(G1CollectedHeap::heap()),
> 163:       _region_idx(_g1h->max_regions()),

I think `_g1h->max_regions()` should be captured in a field named sth like `uninitialized_idx` or `invalid_idx` to make the intend clearer.

src/hotspot/share/gc/g1/g1EvacFailure.cpp line 173:

> 171:         _marked_words += marked_words;
> 172:       } else {
> 173:         _g1h->region_at(_region_idx)->note_self_forwarding_removal_end_par(_marked_words * BytesPerWord);

The `note_self_forwarding_removal_end_par` calls can be placed in a method to avoid some duplication.

src/hotspot/share/gc/g1/g1EvacFailureRegions.cpp line 41:

> 39:   _max_regions(0),
> 40:   _heap(G1CollectedHeap::heap()),
> 41:   _phase_times(_heap->phase_times()) {

I don't really see where this field is used.

src/hotspot/share/gc/g1/g1EvacFailureRegions.hpp line 76:

> 74:   uint max_regions() const {
> 75:     return _max_regions;
> 76:   }

Looking at how this field is used:

1. `region_idx < _max_regions`; can be caught the underlying bitmap
2.  `_max_regions = 0; // To have any record() attempt fail...`; can be replaced by a null-check of `_evac_failure_regions`.

This issue is preexisting though.

src/hotspot/share/gc/g1/g1HeapRegionChunk.hpp line 69:

> 67:   bool include_first_obj_in_region() const { return _include_first_obj_in_region; }
> 68: 
> 69:   bool include_last_obj_in_region() const { return _include_last_obj_in_region; }

Seems unused.

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

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