RFR: 8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper [v2]

[Kim Barrett]

On Mon, 12 Jul 2021 07:58:40 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3534:
>> 
>>> 3532:   Atomic::store(&_num_regions_failed_evacuation, 0u);
>>> 3533: 
>>> 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.

A problem with moving it later to the new placement is that it's now included in the pause time, where it previously wasn't.  I'm not sure it should be included in the pause time, as it's not really part of this collection.  I also wonder if the root scanning ought to be done using the concurrent workers rather than the stw workers, as there are typically fewer of the former (required to not be more?).

-------------

PR: https://git.openjdk.java.net/jdk/pull/4744