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

[Albert Mingkun Yang]

On Wed, 16 Aug 2023 09:44:58 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> 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.

Yes, this looks better. The comment should make it clear whether `is_old_gc_alloc_region` is required for correctness or not. (I believe it is not.)

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

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