RFR: Fix off-by-one error when handling pinned regions in a mixed collection [v2]

[William Kemper]

On Wed, 15 Mar 2023 23:39:50 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> William Kemper has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove over aggressive asserts, improve readability
>
> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 156:
> 
>> 154: 
>> 155:   // Find pinned regions to the left and move their pointer into a slot
>> 156:   // that was pointing at a region that has been added to the cset. We
> 
> (maybe augment this comment: "or was holding another pinned region that has already been copied to an unused slot found further to the right of its original position"

Done.

> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 166:
> 
>> 164:     RegionData& skipped = _region_data[search];
>> 165:     if (skipped._region->is_pinned()) {
>> 166:       RegionData& added_to_cset = _region_data[write_index];
> 
> And this variable might be better named "unused_available_slot" rather than "added_to_cset".  In the case that we are overwriting a pinned region pointer, it was not "added_to_cset".

Yes, that's a good point. I shall also remove the assert for the same reason (it may encounter a slot for a pinned region that has been moved up and such a region should _not_ be in the cset).

> src/hotspot/share/gc/shenandoah/heuristics/shenandoahOldHeuristics.cpp line 186:
> 
>> 184:   //   | Start index maintains the pointer to the beginning of the regions we scanned on this cycle.
>> 185: #ifdef ASSERT
>> 186:   for (size_t check = write_index; check != _start_candidate; --check) {
> 
> I see that _start_candidate is initialized to zero when ShenandoahOldHeuristics is instantiated.  What I do not understand is how _start_candidate is reset to zero each time we rebuild _region_data with a new set of mixed evacuation candidates.  Can you provide a comment to explain this?

That was a bug. I've removed this assertion because we may encounter a slot that refers to a pinned region that was moved up and that should not be in the cset.

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

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