RFR: 8141637: Parallelize single threaded heap region iteration during Pre Evacuate Collection Set
stefan.johansson at oracle.com
Mon Nov 18 16:35:53 UTC 2019
Thanks for reviewing.
On 2019-11-15 11:35, Thomas Schatzl wrote:
> On 15.11.19 10:38, Stefan Johansson wrote:
>> Please review this fix to parallelize parts of the G1 young gc
>> 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
> 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
> 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::_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().
More information about the hotspot-gc-dev