RFR: Handle old, pinned regions [v7]

[Y. Srinivas Ramakrishna]

On Wed, 3 Aug 2022 01:10:57 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> Prior to this change, mixed collections were entirely ignoring the pinned status of old regions. Pinned old regions would not be coalesced and filled (which could lead to a crash) and their pin status was not being synchronized with the pin count (which could also lead to a crash). To address these problems, the following changes have been made:
>> 
>> * Before adding old regions to the collection set, their pinned status is synchronized.
>> * Pinned old regions will _not_ be added to a mixed collection, they will be skipped and considered for inclusion in the next mixed collection.
>> * Any old regions which have not been evacuated after the last mixed collection will be made parseable (coalesced and filled) _at the start of the next old marking cycle_.
>
> William Kemper has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
> 
>  - Merge branch 'shenandoah-master' into fill-old-pinned
>  - Do not drop abbreviated argument when recording success concurrent cycle
>  - Fix whitespace
>  - Prepare for review
>  - Merge branch 'shenandoah-master' into fill-old-pinned
>  - Validate old gen state transitions
>  - Make use of old generation without down casting
>  - Make verifier aware of changes to old generation state machine
>  - Make rset scan aware of changes to old generation state machine
>  - Fix use of index as count and over-counting of garbage
>  - ... and 11 more: https://git.openjdk.org/shenandoah/compare/60e43d04...bdda91d6

One thing I did not manage to figure out is how the work of coalesce and fill is resumed when it is interrupted. May be it can only be canceled and is never resumed after that point? I had expected to see some outer while loop or something repeatedly continuing the unfinished work if it were interrupted.

Thank you for your patient answers as this review was an opportunity to understand some of the code surrounding pinning & coalesce/fill, and their mutual interaction.

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

> 205:     }
> 206: 
> 207:     size_t garbage = region->garbage();

Is `_region_data` above basically a statically allocated scratchpad array that can be used for temporary storage and calculations, and guarantees that it's at least as big as the number of regions in the heap? Typically you use a subset of that array, e.g. in this case you use a prefix for storing collection set candidates.

Is there any sanity checking to ensure that different uses of it do not inadvertently stomp on each others' data? Perhaps there is some state checking somewhere for this in debug/assert mode? Otherwise, it would seem a bit fragile and might pose maintenance headaches down the line.

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

> 210:     if (region->is_regular() || region->is_pinned()) {
> 211:       if (!region->has_live()) {
> 212:         assert(!region->is_pinned(), "Pinned region should have live (pinned) objects.");

This comment pertains to lines 231-234 below for the case where the immediate_regions and immediate_garbage for the humongous regions are being counted. I think while it correctly counts the contribution of the Humongous Continuations, I believe because of the "else if" it misses the contribution of the first Humongous Starts region. 

This isn't a serious issue, however, since it's just a value that is logged rather than being used in guiding any steps/heuristics in the collection. Not your change, but thought I'd mention it.

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

> 272:                _last_old_collection_candidate,
> 273:                byte_size_in_proper_unit(collectable_garbage), proper_unit_for_byte_size(collectable_garbage),
> 274:                byte_size_in_proper_unit(immediate_garbage), proper_unit_for_byte_size(immediate_garbage));

See earlier comment about potentially underreporting `immediate_garbage`. Also, although `immediate_regions` is tracked, it doesn't seem to ever get used anywhere. May be it used to be reported at some point in the past but is no longer? Eliminate its tracking then?

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

Marked as reviewed by ysr (Author).

PR: https://git.openjdk.org/shenandoah/pull/149