RFR: JDK-8262068: Improve G1 Full GC by skipping compaction for regions with high survival ratio [v11]

[Stefan Johansson]

On Fri, 26 Mar 2021 08:26:45 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 43:
>> 
>>> 41: 
>>> 42: bool G1FullGCPrepareTask::G1CalculatePointersClosure::do_heap_region(HeapRegion* hr) {
>>> 43:   bool force_pinned = false;
>> 
>> Unless I'm missing something we could here do a call to a function called something like: `should_compact()` that checks the liveness of the heap region and if it should not be compacted it makes the region pinned() so that the check below will return true and thus avoid compacting it.
>> 
>> Doing this we will have to make sure the regions is "unpinned" at the end of the GC, and that could be handled by the `G1ResetPinnedClosure`. We might need to add a new `HeapRegionType` for this, something like `PinnedForGC`.
>
> I think the current mechanism is fine, just "pin" it internally in that region attribute table. Do not really make the region "pinned" as suggested, that makes things just more complicated as you need to remember which regions are temporarily pinned.
> This also adds another state into `HeapRegionType` for this special case that is already handled well by the region attribute table; the problem is that if we went that way, with region pinning (instead of GCLocker) we may also need to have all combinations of `PinnedForGC` with Eden/Survivor/Old etc.
> 
> Keeping the complexity here (and in that Full GC attribute table), we do not change "global" region state unnecessarily.
> 
> Still I think the code could be cleaned up by first determining that `should_compact` predicate.
> 
> Something like:
> 
>   
>     if (!should_compact(hr)) { // probably it is better to avoid the negation here
>       if (hr->is_humongous()) {
>         ...
>       } else if (hr->is_open_archive())) {
>   
>       } else {
>         // otherwise compacted region, i.e.
>         assert(hr->is_closed_archive() || liveness > threshold)
>       }
>     } else {
>       prepare_for_compaction(hr);
>     }

I'm ok with this, my biggest concern with the above was to have the new logic in the else case. Having the logic in `should_compact()` is the important change. And I agree that:
if (should_compact(hr)) {
  prepare_for_compaction(hr);
} else {
  ...
}
Would read better.

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

PR: https://git.openjdk.java.net/jdk/pull/2760