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

[Albert Mingkun Yang]

On Tue, 13 Sep 2022 12:53:22 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews on this change that makes the evacuation failure remove forwards phase split the work across threads within regions?
>> 
>> This work is a continuation of PR#7047 from @Hamlin-Li and latest improvements from @albertnetymk in that thread. This is reflected in the first commit, I added a fair amount of changes to fix bugs and streamline the code.  
>> 
>> Testing: tier1-8 with G1EvacuationFailureALot enabled, tier1-5 as is
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   sjohanns, ayang review 2

Instead of calling `note_end_of_clearing` in `G1ConcurrentMark::clear_bitmap_for_region`, I wonder if moving it (`note_end_of_clearing`) to a higher-level caller (`G1ClearBitmapClosure`) is a bit nicer. This way, `G1ConcurrentMark::clear_bitmap_for_region` does exactly what its name suggests, nothing more&less.

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

> 3301: }
> 3302: 
> 3303: void G1CollectedHeap::mark_evac_failure_object(uint worker_id, const oop obj, size_t obj_size) const {

Can we assert the obj is _not_ marked as a pre-condition?

src/hotspot/share/gc/g1/g1EvacFailure.hpp line 45:

> 43:   CHeapBitMap _chunk_bitmap;
> 44: 
> 45:   // Initialized outside of the constructor because the number of workers is unknown

This comment is obsolete I believe.

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp line 117:

> 115: 
> 116:     return ((double)G1CollectedHeap::get_chunks_per_region() / G1RestoreRetainedRegionChunksPerWorker) *
> 117:       _evac_failure_regions->num_regions_failed_evacuation();

I think the following is a bit cleaner.

size_t total_chunks = get_chunks_per_region() * _evac_failure_regions->num_regions_failed_evacuation();
return (double) total_chunks / G1RestoreRetainedRegionChunksPerWorker;

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp line 138:

> 136:   add_parallel_task(G1CollectedHeap::heap()->rem_set()->create_cleanup_after_scan_heap_roots_task());
> 137:   if (evacuation_failed) {
> 138:     RestoreRetainedRegionsTask* restore_retained_regions_task = new RestoreRetainedRegionsTask(evac_failure_regions);

`restore_retained_regions_task` is not really used/needed, right?

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

PR: https://git.openjdk.org/jdk/pull/9980