RFR: 8349688: Crash assert(!_g1h->heap_region_containing(p)->is_survivor()) failed: Should have filtered out from-newly allocated survivor references already [v2]

[Thomas Schatzl]

On Tue, 18 Feb 2025 09:12:50 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Hi,
>> 
>> Please review this fix to the bug in setting a region index to the optional cset. The crash happens because the incorrect `_index_in_opt_cset` refers to a region that has already been evacuated.
>> 
>> Testing: tier5-common-apps
>
> Ivan Walulya has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into JDK-8349688-OptionalRegions
>  - set_index_in_opt_cset correctly

Seems good, thanks for catching this.

However, it would be nice to rename the PR and CR to something understandable, like "G1: Wrong initial optional region index when selecting candidates from retained regions" or so.

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

> 484: void G1CollectionSet::select_candidates_from_retained(double time_remaining_ms) {
> 485:   uint num_initial_regions = 0;
> 486:   uint prev_num_optional_regions = _optional_groups.num_regions();

It would be great if the code in `G1CollectionSet::select_candidates_from_marking` would check that the number of optional regions is zero as well when initializing its `num_optional_regions`.

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

PR Review: https://git.openjdk.org/jdk/pull/23568#pullrequestreview-2623141503
PR Review Comment: https://git.openjdk.org/jdk/pull/23568#discussion_r1959497709