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

[Thomas Schatzl]

On Wed, 15 Feb 2023 16:23:31 GMT, Ivan Walulya <iwalulya 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.
>> 
>>> Common case, the number of regions in serial == ~no of threads
>> 
>> True for now, but when humongous regions can be moved, this number can go up.
>
>> I think what I suggested requires less code than the sorting approach.
> 
> Unfortunately, that approach is only valid if I add the bitmap as suggest by @kstefanj, otherwise you may iterate over all regions yet the serial compaction has just a few regions as for the test-case I highlighted above.

> > 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).

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

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