RFR: 8306541: Refactor collection set candidate handling to prepare for JDK-8140326 [v5]

[Thomas Schatzl]

On Tue, 9 May 2023 09:38:07 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> (After deleting the other message)
>> There is a use in `G1CollectionSetRegionList::remove` where not having this value would add a loop over the `other` list. If you think it is really important, I can change that.
>> I simply do not think it hurts, and avoids the additional iteration (as `reclaimable_bytes` is calculated during appending to that list, which is done iteratively already).
>
>> would add a loop over the other list
> 
> I don't get it.
> 
> 
> void G1CollectionSetRegionList::remove(G1CollectionSetRegionList* other) {
> #ifdef ASSERT
>   // Check that the given list is a prefix of this list.
>   int i = 0;
>   for (HeapRegion* r : *other) {
>     assert(_regions.at(i) == r, "must be in order, but element %d is not", i);
>     i++;
>   }
> #endif
> 
>   if (other->length() == 0) {
>     return;
>   }
>   _regions.remove_till(other->length());
>   _reclaimable_bytes -= other->reclaimable_bytes();
> }
> 
> 
> If one removes `_reclaimable_bytes`, the last statement will go away. Why do you need an additional loop?

I stand corrected :) Removed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13666#discussion_r1188399199