RFR: 8329528: G1 does not update TAMS correctly when dropping retained regions during Concurrent Start pause

[Thomas Schatzl]

On Wed, 10 Apr 2024 11:44:34 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> The fix looks good. However, in the long run, pulling out the logic around `G1NumCollectionsKeepPinned` from the cset construction seems cleaner.

That is true at first glance, but when trying that out for this change, to me the drawbacks outweighed the cleanliness argument that resulted in much more code:
  * extra loop over the collection set candidates, where that loop is at a different place in the code, called from `pre_evacuate_collection_set`. This creates an extra additional dependency when tracking dependencies there. One could imagine to add time tracking for this phase as well, resulting in more extra code.
  * logging of this filtering will become disjointed from collection set selection. Dropping a region from the collection set candidates implies not selecting it, and reflecting this properly in the selection process takes additional effort unless one agrees with having separate log messages. (E.g. it is a bit surprising that when you know that there were pinned regions, the cset selection may actually tell you that there were none).
  * this is very little extra code piggybacking it on the collection set selection

I think I considered this extra step for filtering when implementing this the first time, rejected it for all the extra effort (probably in the range of 100 LOC, see the diff for the https://github.com/openjdk/jdk/pull/18692/commits/0c46d6a653365472421123f021cda414834fac15 change - which is incomplete btw as it does not adjust the regression test and potentially add proper timing/logging) just to basically avoid this single `if` clause there.

Anyway, modifying this in this change at least it detracts from the problem at hand. I will file an RFE.

>Also, `NoteStartOfMarkHRClosure` can keep the original simpler condition.

The additional clause is an arbitrary requirement/assumption by the existing code, asserting somewhere that cset regions do not have tams set initially.
All code paths for regions in the collection set update the TAMS, either clear again when freeing the region, or set it properly during evacuation failure, afair. I wanted to keep the initial assumption of cset regions having tams == bottom throughout.

Could you approve the change if it looks good to you?

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

PR Comment: https://git.openjdk.org/jdk/pull/18692#issuecomment-2049195120