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

[Stefan Johansson]

Hi Thomas,

Thanks for reviewing.

On 2019-11-15 11:35, Thomas Schatzl wrote:
> 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"
I agree, done.

> 
> 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?
Fixed.

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

> 
> - 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).
Updated prepare_region_for_scan to take these three comments into 
account, I think this really makes sense.

> 
> - 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.
Done.
> 
> - 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().
> 
Oups, removed.

Updated webrevs:
Full: http://cr.openjdk.java.net/~sjohanss/8141637/01/
Inc: http://cr.openjdk.java.net/~sjohanss/8141637/00-01/

Thanks,
Stefan

> Thanks,
>    Thomas