RFR: Handle old, pinned regions [v5]

[Y. Srinivas Ramakrishna]

On Thu, 4 Aug 2022 22:18:04 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 47:
>> 
>>> 45:   // for pinned regions.
>>> 46: 
>>> 47:   // This points to the first candidate of the current mixed collection. This
>> 
>> There is an implied order in first, last, etc. but the criterion for the ordering isn't explained here. (May be it is later.) I think the comment would read better if the high level idea of maintaining this ordered set of candidates was made clear at the outset, and the criteria underlying the order.
>
> They're ordered from most to least garbage. I'll call that out explicitly in the comments.

Upon first reading, I had trouble figuring out what array these numbers were being used as indexes into. It would help to state that in a comment up fromt. Perhaps something like:

"Candidate regions for evacuation are maintained in the _region_data array. The following members are indexes into the array and help with tracking candidate regions, including pinned regions which may not be immediately available for evacuation. When ready they are sorted in order of decreasing garbage. Pinned regions are skipped and accumulate to the left of ... Pinned regions may become unpinned and are then available for evacuation.... Any candidates that were not evacuated before the commencement of a new marking cycle have their garbage payload maximally coalesced and overlaid by fillers to prevent scanning or walking dead objects."

>> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.hpp line 53:
>> 
>>> 51:   // Pinned regions may not be included in the collection set. Any old regions
>>> 52:   // which were pinned at the time when old regions were added to the mixed
>>> 53:   // collection will have their pointers shifted down so that they are at the
>> 
>> "have their pointers shifted down" is not clear without more context. Nit: Also "pointers" sounds out of place here. You can identify the region pointers with the regions for the purpose of documentation comments here.
>> 
>> Also, I'd pick one consistent terminology, may be first and last (by index), rather than down and up, or left and right, which are more ambiguous (unless it's made clear).
>
> s/their pointers/the pointers to them ? In my head, I was mostly thinking in terms of left (most garbage) and right (least garbage). I will update the variables and comments for consistency.

I was suggesting dropping "pointers" entirely, but just talk about regions. But this, like many of my comments about documentation above, are quite subjective. So, please use your taste and judgement in these cases.

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

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