RFR: 8364925: G1: Improve program flow around incremental collection set building [v2]

[Albert Mingkun Yang]

On Thu, 7 Aug 2025 15:05:39 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1CollectionSet.cpp line 133:
>> 
>>> 131: void G1CollectionSet::start_incremental_building() {
>>> 132:   assert(_collection_set_cur_length == 0, "Collection set must be empty before starting a new collection set.");
>>> 133: 
>> 
>> Can we assert `selected_groups_cur_length() == 0` also? If so, then maybe it's best to inline `continue_incremental_building` with explicit zero-setting.
>
> Also adding `_optional_groups().length() == 0` here. I think calling `continue_increment...` is just fine; first start the collection set building as a whole, then start a new (first) increment within that. Maybe the name `continue_...` should be improved?

I think it's easier to read with the inlined version -- those operations are not well abstracted to get a meaningful name.

>> src/hotspot/share/gc/g1/g1CollectionSet.hpp line 152:
>> 
>>> 150:   G1CSetCandidateGroupList _collection_set_groups;
>>> 151: 
>>> 152:   uint selected_groups_cur_length() const;
>> 
>> Maybe move this down to methods section so that all fields are next to each other.
>
> I wanted to have this there to correspond to the place for the similar member for the regions.

I find it a bit odd mixing fields and methods. YMMV.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26659#discussion_r2260640984
PR Review Comment: https://git.openjdk.org/jdk/pull/26659#discussion_r2260674977