RFR: 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 [v2]

[Albert Mingkun Yang]

On Mon, 30 Oct 2023 13:25:07 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> src/hotspot/share/gc/g1/g1EvacFailureRegions.hpp line 36:
>> 
>>> 34: class HeapRegionClaimer;
>>> 35: 
>>> 36: // This class records for every region on the heap whether it has to be retained
>> 
>> I feel the term "retain" has two diff meanings in this PR:
>> 
>> 1. retain == pinned or evac-fail
>> 2. should_retain_evac_failed_region
>> 
>> 1 is during scavenging while 2 is after scavenging.
>
> Maybe rename `should_retain_evac_failed_region` to `should_keep_retained_region[_in_old]` or something?

Is it possible to drop 1 so that an obj is evac-fail iff it's pinned or OOM? (I feel "retain" is on region-level.)

>> src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp line 82:
>> 
>>> 80:   } else {
>>> 81:     assert(hr->containing_set() == nullptr, "already cleared by PrepareRegionsClosure");
>>> 82:     if (hr->has_pinned_objects() ||
>> 
>> This `do_heap_region` method is hard to follow; there multiple occurrences of same predicates. I wonder if one can reorganize these if-else a bit. Inlining `should_compact` should make all `if` on the same level at least.
>
> Apart from having an early return in the `should_compact`-if, one option would be making `has_pinned_objects()` more clever by stating something like:
> 
> 
>   bool has_pinned_objects() const {
>     return pinned_count() > 0 || (is_continues_humongous() && humongous_start_region()->pinned_count() > 0);
>   }
> 
> 
> Then this predicate would get shorter. Or add a local helper for that (as suggested in the next commit). Either is fine with me.

A local helper is possibly clearer here, IMO.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376247318
PR Review Comment: https://git.openjdk.org/jdk/pull/16342#discussion_r1376251206