RFR: 8352969: G1: Improve testability of optional collections [v7]

[Thomas Schatzl]

On Mon, 1 Sep 2025 15:38:18 GMT, Guanqiang Han <ghan at openjdk.org> wrote:

>> This PR introduces a new diagnostic flag EvacuateAllOptionalRegions to force G1 GC to evacuate all optional regions regardless of the predicted pause time. The motivation is to allow testing and validation of optional region evacuation behavior without being constrained by the remaining pause time.
>
> Guanqiang Han has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
> 
>  - change flag name
>  - Merge remote-tracking branch 'upstream/master' into 8352969
>  - fix format error
>  - format fix
>  - Optimize implementation and add regression test
>  - Merge remote-tracking branch 'upstream/master' into 8352969
>  - Update g1YoungCollector.cpp
>    
>    format fix
>  - Simplified implementation
>  - add default value
>  - Implement JDK-8352969

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 407:

> 405:   G1CSetCandidateGroupList* from_marking_groups = &candidates()->from_marking_groups();
> 406: 
> 407: #ifndef PRODUCT

The correct ifdef to use is `#ifdef ASSERT` for `develop` flags. The `optimized` build (actually not sure it still exists) does not define either PRODUCT or ASSERT

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 408:

> 406: 
> 407: #ifndef PRODUCT
> 408:   bool add_at_least_one_optional_region = G1EvacuateAllOptionalRegions;

Maybe:
Suggestion:

  bool make_first_group_optional = G1EvacuateAllOptionalRegions;

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 431:

> 429:         _optional_groups.append(group);
> 430:         prepare_optional_group(group, num_optional_regions);
> 431:         num_optional_regions += group->length();

Please factor out this code with the one below adding optional groups. This also seems to be missing the timing update.

src/hotspot/share/gc/g1/g1CollectionSet.cpp line 666:

> 664:   if (G1EvacuateAllOptionalRegions && num_regions_selected == optional_regions_count) {
> 665:     log_debug(gc, ergo, cset)("All optional regions are scheduled to be evacuated");
> 666:   }

Isn't it that "all selected regions are optional regions"? I am not sure we need this message, this content can be deduced from above.

I understand that it is easier to check for in the test, but doing a regexp and comparing the values is only slightly more complicated imo.

src/hotspot/share/gc/g1/g1_globals.hpp line 375:

> 373:   develop(bool, G1EvacuateAllOptionalRegions, false,                        \
> 374:           "Force to evacuate all optional regions.")                        \
> 375:                                                                             \

Suggestion:

  develop(bool, G1ForceOptionalEvacuation, false,                        \
          "Force optional evacuation for all GCs where there are old gen collection set candidates. Also schedule all available optional groups for evacuation regardless of timing.")                        \
                                                                            \


(Please distribute the new comment properly over multiple lines)

I think we should move away from optional "regions" because the unit of evacuation is always groups now (even if they only contain one region).

test/hotspot/jtreg/gc/g1/TestOptionalRegionGC.java line 27:

> 25:  * @bug 8352969
> 26:  * @summary Verify that the G1EvacuateAllOptionalRegions flag forces G1
> 27:  *          to evacuate all of optional regions, improving testability.

Suggestion:

 * @summary Verify that the G1EvacuateAllOptionalRegions flag forces G1
 *          to evacuate all of optional regions.


Or just: "Tests optional evacuation."

For the same reasons as indicated for the flag name, it should not contain the term "regions".

test/hotspot/jtreg/gc/g1/TestOptionalRegionGC.java line 56:

> 54:       "-XX:+UseG1GC",
> 55:       "-XX:MaxTenuringThreshold=1",
> 56:       "-Xlog:gc+ergo+cset=trace",

I would add a `-XX:+VerifyAfterGC` here to make sure the optional evacuation leaves the heap in a correct state.

test/hotspot/jtreg/gc/g1/TestOptionalRegionGC.java line 85:

> 83:       wb.youngGC();
> 84:       // Clear certain references for mixed GC.
> 85:       for (int i = 0; i < NUM_OBJECTS; i=i+2) {

Suggestion:

      for (int i = 0; i < NUM_OBJECTS; i+=2) {

(Or just spaces about the operator and `=`)

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

PR Review: https://git.openjdk.org/jdk/pull/26880#pullrequestreview-3201495820
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333529097
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333592238
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333537363
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333542748
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333616489
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333559784
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333563725
PR Review Comment: https://git.openjdk.org/jdk/pull/26880#discussion_r2333571110