RFR: 8302215: G1: Last-ditch Full GC should do serial compaction for tail regions in per thread compaction points. [v2]

[Stefan Johansson]

On Wed, 15 Feb 2023 19:46:42 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>>> > Fwiw, I tend to prefer the sort call as it is least amount of code that achieves the goal well enough. All these other suggestions seem to be optimizations for little gain that need lots of extra code/helper data structures for something not (time) critical.
>>> 
>>> I think what I suggested requires less code than the sorting approach.
>>> 
>> 
>> I probably did not understand the pseudo code enough; thinking about it some more, it might work;
>> 
>> The first step is basically dropping all regions >= `lowest_current_hr`; the second about rebuilding the serial cp regions from scratch given the region list (and using some of the existing conditions on whether to add the region to the final serial cp).
>> 
>> The following comes to my mind about this:
>> 
>> * you should not linearly iterate over the discontiguous range of regions using region indexes; there is the public `region_at_or_null` in CollectedHeap to ignore unavailable regions but it's kind of a "don't use" method; code at this level should preferably be oblivious about the regions being contiguous in the heap and holes in the region map.
>> The only code that uses it this way (`G1RebuildFreeListTask`) is internal to the `HeapRegionManager` (others are to simplify some code or asserts, so maybe that `region_at_or_null` method should be made internal).
>> 
>> * `G1CollectedHeap::heap_region_iterate()` that can be used instead does not guarantee an order of iteration of regions. The current implementation does give you regions in address order though. Not really happy on starting to rely on order of iteration in `heap_region_iterate()` - I am not aware of any other closure doing that (apart of that internal `G1RebuildFreeListTask` which is essentially part of `HeapRegionManager`)
>> 
>> Not sure how I feel about breaking these fairly basic assumptions in how G1 code should think about the heap (particularly the first one).
>
> Actually, the existing full gc code probably already breaks the second assumption already when handing out regions round-robin, i.e. assuming that we get regions in increasing address order (but the code would still work without that I guess).

I don't have a strong opinion on how to solve this, but I agree with Albert. I think adding a bitmap to keep track of what regions to put in the serial cp and removing them from the others would be less code. But I might be missing some case. I don't see this as an optimization I see it as a more straight forward solution. But we all have different tastes on how to solve things. So I'm good if you want to keep the sort.

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

PR: https://git.openjdk.org/jdk/pull/12529