RFR: 8314100: G1: Improve collection set candidate selection code

[Thomas Schatzl]

On Fri, 11 Aug 2023 14:14:17 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>>   discussions during review of [JDK-8140326](https://bugs.openjdk.org/browse/JDK-8140326) indicated that he code surrounding selecting candidates after marking/rebuild for the collection set could be improved.
>> 
>> Testing: gha
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/gc/g1/g1CollectionSetChooser.cpp line 161:
> 
>> 159:         !G1CollectedHeap::heap()->is_old_gc_alloc_region(r) &&
>> 160:         G1CollectionSetChooser::region_occupancy_low_enough_for_evac(r->live_bytes()) &&
>> 161:         r->rem_set()->is_complete();
> 
> Does this work?
> 
> 
> assert(!r->rem_set()->is_updating(), "inv");
> if (!r->rem_set()->is_complete()) {
>   return false;
> }
> if (G1CollectionSetChooser::region_occupancy_low_enough_for_evac(r->live_bytes())) {
>   add_region(r);
> } else {
>   r->rem_set()->clear(true /* only_cardset */);
> }

I do not think so. Old GC alloc regions need special handling either way (unregistering them as such), and the suggested code may keep them as candidates - they may have a (complete) remembered set here (unless you suggest to change the meaning).

Not sure if breaking up the conjunction any further improves the code; the breakout of the earlier `if (is_old() || !...)` avoids the repetition for the remset clearing.

Something like

assert(!r->rem_set()->is_updating(), "must be");
if (!r->rem_set()->is_complete()) {
  return false;
}
bool should_add = !r->is_old_gc_alloc_region() || G1CollectionSetChooser::region_occupancy_low_enough_for_evac(r->live_bytes())
if (should_add) {
  add_region(r);
} else {
  r->rem_set()->clear(true /* only_cardset */);
}

would work if that is better for you.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15243#discussion_r1295648467