RFR: 8140326: G1: Consider putting regions where evacuation failed into next collection set [v4]

[Thomas Schatzl]

On Wed, 2 Aug 2023 10:55:07 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> It can, see a prototype at https://github.com/openjdk/jdk/compare/pr/14220...tschatzl:jdk:8140326-evacuate-retained-regions-at-any-time-work?expand=1 ; would that change be what you had in mind?
>
> Yes; the change in `note_start_of_marking` is surprising though. Why do regions in cset need tams? I thought marking doesn't touch cset.

Adding the `&& !in_collection_set()` does not help avoiding that unconditional setting of TAMS in the post evacuate phase (line 362 in [g1YoungGCPostEvacuateTasks.cpp](https://github.com/openjdk/jdk/compare/pr/14220...tschatzl:jdk:8140326-evacuate-retained-regions-at-any-time-work?expand=1#diff-6cb4285ce6a3b0e737e0612a2fd393a54f9c841a6be16f4ac58a097fdd69f914)`), only complicates code, so I removed it. I can re-add it.

The real reason are optional collection set regions: we do not know yet whether they will be evacuated or not, so there is the option to either set their tams eagerly or not. If not, we would need to add extra code to set their tams somewhere (add at least one of the "if (in-conc-mark)" checks when clearing the optional region list - a check that you disliked earlier).
That would kind of defeat the simplicity of "set tams for all regions at concurrent mark start pause" (which already got quite complicated to me btw).

I implemented a prototype for that last version in https://github.com/openjdk/jdk/commit/d146bdcefb345085bc968c929ca9c0fc8b26db09 (untested).

The downside of having that single line in `g1YoungGCPostEvacuateTasks.cpp` vs. being most precise seems to be negligible.

Tell me what version you prefer.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14220#discussion_r1281849290