RFR: 8329790: GenShen: Old regions that are pinned during final mark are not made parseable [v2]

[Kelvin Nilsen]

On Fri, 12 Apr 2024 22:57:23 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Pinned regions that are not humongous may be added to the set of _candidates_ for mixed collection. They still may not be added to the collection set for a mixed collection. Regions that are not in this collection of _candidates_ will not be coalesced and filled.
>
> William Kemper 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 six additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'shenandoah/master' into coalesce-old-pinned-regions
>  - Use is_regular_pinned to restore candidate selection logic to its previous form
>  - Introduce 'regular pinned' state
>  - Merge remote-tracking branch 'shenandoah/master' into coalesce-old-pinned-regions
>  - Allow pinned/regular regions into mixed collection candidates
>  - Little clean up

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 327:

> 325:     live_data += live_bytes;
> 326: 
> 327:     if (region->is_trash()) {

In revised code structure, we handle trash regular regions with same code that handles trash humongous regions.  I like this improvement.  This is a note to self.  Please let me know if I am misunderstaning.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 331:

> 329: 
> 330:     // Only place regular regions into the candidate set
> 331:     if (region->is_regular()) {

I understand that the rationale for the PR is to make sure we do not overlook pinned regions.  What I'm not seeing is how the original code is excluding the pinned regions from being inserted into the "old collection set".  To help my review, can you point me to that part of the original code?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 332:

> 330:       immediate_garbage += garbage;
> 331:     } else if (region->is_humongous_start()) {
> 332:       // This will handle humongous start regions whether they are also pinned, or not.

Since we've already handled region->is_trash() above, do we need this code?  If we want to preserve the assert(!is_pinned), we can insert that into the is_trash() code above.  Right?

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 347:

> 345:       }
> 346:     } else if (region->is_regular() || region->is_pinned()) {
> 347:       if (!region->has_live()) {

Here, I'm not sure why we ask "|| region->is_pinned()".  We don't want humongous regions that are pinned.  And we do want regular regions whether or not they are pinned.  Also, I think it better to handle the case of !region->has_live() at line 327 above.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 443:

> 441:            (total_uncollected_old_regions < 15 * span_of_uncollected_regions / 16)) {
> 442:       ShenandoahHeapRegion* r = candidates[_last_old_collection_candidate]._region;
> 443:       assert((r->is_regular() || r->is_pinned()) && !r->is_humongous(), "Region " SIZE_FORMAT " has wrong state for collection: %s",

I think assert should be r->is_regular(). (testing for r->is_pinned() and !r->is_humongous() seems redundant and unnecessary).  Or explain the rationale for assert as written with comment.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 469:

> 467:   log_info(gc)("Old-Gen Immediate Garbage: " PROPERFMT " over " SIZE_FORMAT " regions",
> 468:               PROPERFMTARGS(immediate_garbage), immediate_regions);
> 469:   log_info(gc)("Old regions selected for defragmentation: " SIZE_FORMAT, defrag_count);

Might be good to add a comment here that defrag_count is a subset of old_candidates.  defrag_count represents regions that were placed into the old collection set in order to defragment the memory that we try to "reserve" for humongous allocations.  old_candidates includes defrag_count, plus it also includes old-gen regions that are to be collected because they contain an abundance of garbage.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554451689
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554459766
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554452329
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554457264
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554450956
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554458847