RFR: 8343782: G1: Use one G1CardSet instance for multiple old gen regions [v8]

[Ivan Walulya]

On Tue, 17 Dec 2024 09:56:48 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Ivan Walulya has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix space issues
>
> src/hotspot/share/gc/g1/g1CollectionSetCandidates.hpp line 42:
> 
>> 40: struct G1CollectionSetCandidateInfo {
>> 41:   G1HeapRegion* _r;
>> 42:   double _gc_efficiency;
> 
> Seems that this field has become unused.

Fixed

> src/hotspot/share/gc/g1/g1HeapRegion.cpp line 155:
> 
>> 153:   // rely on the predition for this region.
>> 154:   if (_rem_set->is_added_to_cset_group() && _rem_set->cset_group()->length() > 1) {
>> 155:     return -1.0;
> 
> I believe all special cases logic (returning -1`) in this method belong to the caller, `G1PrintRegionLivenessInfoClosure`, where we branch use `if(gc_eff < 0) {`.

FIxed

> src/hotspot/share/gc/g1/g1HeapRegionRemSet.hpp line 56:
> 
>> 54:   // nullptr guards before every use of _cset_group.
>> 55:   G1CSetCandidateGroup* _default_cset_group;
>> 56:   G1CSetCandidateGroup* _cset_group;
> 
> As I understand it, only one of these two fields contains the real group. I don't get why we need null-checks if only `_cset_group` is there. Whenever we work with `_cset_group`, we should know whether it's null or not already depending on the call-site.

Refactored to guarantee that all call-sites are aware of this detail, then removed the _default_cset_group.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1893213194
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1893213277
PR Review Comment: https://git.openjdk.org/jdk/pull/22015#discussion_r1893214413