RFR: 8141637: Parallelize single threaded heap region iteration during Pre Evacuate Collection Set

[Thomas Schatzl]

Hi,

On 15.11.19 10:38, Stefan Johansson wrote:
> Hi,
> 
> Please review this fix to parallelize parts of the G1 young gc preparation.
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8141637
> Webrev: http://cr.openjdk.java.net/~sjohanss/8141637/00/
> 
> Summary
> To sub-phases of the "Pre Evacuate Collection Set" phase, "Region 
> Register" and "Prepare Heap Roots" previously did single threaded 
> iteration of all regions in the heap. For heaps with a lot of regions 
> this become a problem so this change groups these iterations into one 
> task that is executed in parallel.
> 
> Testing
> Manual performance testing show good results on large heaps and no 
> problem on small heaps. Functional tests using mach5 tier 1-4.
> 

A few nits:

- G1RemSet::prepare_region_for_scannning: for naming consistency I would 
prefer "prepare_region_for_scan"

Also there is now a near-name clash with 
prepare_for_scan_heap_roots(uint region_idx). Maybe rename the latter to 
"disable_scan_for_region" or something similar (which is a pre-existing 
bad name), with a better comment?

- please move the comment for G1RemSet::prepare_region_for_scanning() in 
the cpp file to the hpp file.

- existing: in G1RemSet::prepare_region_for_scanning: please add a 
comment in the code path for regions in the collection set that "we do 
not need to disable scanning for these regions as the default is to not 
scan".

Actually I think it is useful to move the clear_scan_top() call for all 
regions to here from G1RemSetScanState::prepare(), which makes it clear 
that we reset it for all regions here. (Obviating the need for this 
comment).

Also the remaining reseet of 
G1RemSetScanstate::_collection_set_iter_state from 
G1RemSetScanstate::prepare could be moved here (or calling a method that 
initializes a given region in G1RemSetScanState).

- move the declaration of G1RemSet::prepare_region_for_scanning() below 
cleanup_after_scan_heap_roots. The comment of 
prepare_for_scan_heap_roots() references the cleanup method, as 
following the prepare method. I.e. the new method declaration is 
confusingly placed inbetween.

- unnecessary newlines: before 
G1PrepareRegionsClosure::humongous_region_is_candidate(), in 
G1PrepareEvacuationTask::G1PrepareEvacuationTask() (probably just 
collapse the end bracket in the same line as the opening bracket). After 
the closing bracket of G1PrepareRregionsClosure::do_heap_region().

Thanks,
   Thomas