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

[William Kemper]

On Wed, 10 Apr 2024 15:12:05 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

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

I agree, it's confusing to have `is_pinned` mask the underlying state of the region.

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

I'm adding a `is_regular_pinned` method to `shHeapRegion`.

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

PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1563239639
PR Review Comment: https://git.openjdk.org/shenandoah/pull/420#discussion_r1563237822