RFR: 8270009: Factor out and shuffle methods in G1CollectedHeap::do_collection_pause_at_safepoint_helper

Kim Barrett kbarrett at openjdk.java.net
Sun Jul 11 22:38:58 UTC 2021

On Fri, 9 Jul 2021 14:40:50 GMT, Thomas Schatzl <tschatzl 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
> Thanks,
>   Thomas

A few minor nits, but generally looks good.  Hopefully I didn't miss anything; this kind of code rearrangement can be hard to review.  (Not that it shouldn't be done, but all the scrolling around and needing to dig through things made it hard to keep track.)

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2992:

> 2990: void G1CollectedHeap::set_default_active_worker_threads(){
> 2991:   uint active_workers = WorkerPolicy::calc_active_workers(workers()->total_workers(),
> 2992:           workers()->active_workers(),


src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3020:

> 3018:   ResourceMark rm;
> 3019: 
> 3020:   IsGCActiveMark active_gc_mark;

Moving the IsGCActiveMark here expands the scope of is_gc_active() being true to cover areas it didn't used to.  I _think_ this is okay, but I didn't check all callers carefully.

src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 3050:

> 3048:       {
> 3049:         // The elapsed time induced by the start time below deliberately elides
> 3050:         // the possible verification above.

Why is this comment being removed?

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.


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

More information about the hotspot-gc-dev mailing list