On Mon, 30 Oct 2023 13:25:07 GMT, Thomas Schatzl <tscha...@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
