RFR: 8141637: Parallelize single threaded heap region iteration during Pre Evacuate Collection Set
thomas.schatzl at oracle.com
Fri Nov 15 10:35:08 UTC 2019
On 15.11.19 10:38, Stefan Johansson wrote:
> 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/
> 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.
> 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
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
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
Also the remaining reseet of
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
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().
More information about the hotspot-gc-dev