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

[Kelvin Nilsen]

On Sat, 6 Apr 2024 00:31:10 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> 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.

Need to double-check myself here.  is_trash() may not be exactly the same as !region->has_live().  If these are distinct, maybe we want to check for either condition right here.

> 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?

No need for this.  I now understand that is_regular() does not include is_pinned_regular().

> 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.

I get it now.  But would prefer that we rename these region-state accessors as proposed above.  I've discovered after more thorough testing of the customer workload that we will occasionally also encounter old pinned_cset!  That would represent an unintentional match to the guard for this conditional code.  See draft https://github.com/openjdk/shenandoah/pull/414 for how I've handled this on my development and test branch.

I'd like for your PR to go in first, and then I'll merge any additional (refactored) changes from PR414.  I expect I'll be dividing PR414 into fix-coalesce-and-fill and improvements to ShenandoahVerify and maybe something else...

> 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.

After better understanding of the region state attributes, I now understand how this code works.  Refecting on how this bug came to exist, I think part of the "root cause" is inconsistency in how these attributes are named and accessed.  Conceptually, pinned and "region type" are orthogonal. We can have pinned humongous and pinned regular and pinned cset.  The naming of RegionState enum constants is inconsistent:

1. We have pinned_cset, pinned_humongous_start, but not pinned_regular.  Instead, pinned means pinned_regular.  Would it not be better to replace pinned with pinned_regular?
2. Also, we have is_humongous_start() which is defined as (state == humongous_start || state == pinned_humongous_start)
3. We have is_cset(), defined as (state == cset || state == pinned_cset)
4. But we have is_regular() which does not include state == pinned_regular.  Wouldn't it be better to define is_regular() as (state == regular || state == pinned_regular).  Making this change would require scrutiny of every existing use of is_regular() and revise as appropriate

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1554454748
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1559634302
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1559632710
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1559614470