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

Stefan Johansson stefan.johansson at oracle.com
Mon Nov 18 16:35:53 UTC 2019

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?

> - 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).
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.
> - 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,
>    Thomas

More information about the hotspot-gc-dev mailing list