RFR: 8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper
tschatzl at openjdk.java.net
Mon Jul 12 08:03:02 UTC 2021
On Sun, 11 Jul 2021 22:31:54 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>> Hi all,
>> can I get reviews for this change that factors out a few methods from `do_collection_pause_at_safepoint_helper` and reshuffles code from `gc_prologue`/`gc_epilogue` to be able to put those into parallel phase(s) later.
>> Testing: tier1-5
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3534:
>> 3532: Atomic::store(&_num_regions_failed_evacuation, 0u);
>> 3534: wait_for_root_region_scanning();
> I kind of dislike having this wait moved here into this helper function, where I feel like it's less obvious in the flow of the collection pause. I'd rather it was still in the main body. And it's not like there's anything gained by waiting until after the other things that precede it in this function. It looks like it's okay to delay the wait later than its current location though; the scan is performed using the concurrent marking threads, so other parts of the setup along the way should be fine.
I came to the same conclusion and considering that waiting as a candidate for putting it into the going-to-be parallel pre-evacuation phase. Then again, it might not be useful to have both the concurrent worker threads and parallel threads working at the same time (for performance reasons only; I do not consider the potential parallel verification also using the parallel worker threads as an issue). I checked that verification does not interfere with it (i.e. verification does not care about the mark bitmap in that sense).
The only requirement I can see is that all of these root-region objects need to have their referents marked, i.e. they must not be overwritten in some way, which this place guarantees as well, and to me it seems a more fitting place.
More information about the hotspot-gc-dev