RFR: 8306836: Remove pinned tag for G1 heap regions

[Thomas Schatzl]

On Tue, 25 Apr 2023 15:05:10 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Hi all,
>> 
>>   please review this change that removes the pinned tag from `HeapRegion`.
>> 
>> So that "pinned" tag for G1 heap regions indicates that the region should not move during (young) gc. This applies to now removed archive regions and humongous objects/regions.
>> 
>> With "real" g1 region pinning to deal with gclocker in g1 once and for all upcoming we need a refcount, a single bit is not sufficient anymore. Further there will be a naming conflict as this kind of "pinning" is different to g1 region pinning "pinning". The former indicates "contents can not be moved, but can be reclaimed", while the latter means "contents can not be moved and not reclaimed".
>> 
>> The (current) pinned flag is surprisingly little used, only for policy decisions.
>> 
>> The suggestion this change implements is to remove the "pinned" tag as it is, and reserve it for future g1 region pinning (that needs to store the pinning attribute differently as a refcount anyway).
>> 
>> Testing: tier1-3, gha
>> 
>> Thanks,
>>   Thomas
>
> src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 414:
> 
>> 412:       // There are no other valid region types. Check for one invalid
>> 413:       // one we can identify before crashing: non-movable.
>> 414:       assert(hr->is_young_gc_movable(), "Heap region %u is non-movable.", hr->hrm_index());
> 
> Given this branch is unreachable, can one just use `fatal(...)` without a predicate?

>The name, is_young_gc_movable, seems to suggest it is context sensitive. Maybe it's clearer if it's not defined as an API of heap-region, like other region-type-predicates.

It is defined in `G1Policy`, there is simply a shortcut via `HeapRegion`. I considered removing that shortcut in `HeapRegion`, but then this would mean lots of clutter everywhere to get to the policy object.

I will find a better location for a helper.

>
>For example, it's odd to see them on the same abstraction level, when the two APIs refer to sth quite different.
>
>  assert(hr->is_young_gc_movable(), "Should only be movable region in compaction queue");
>  assert(!hr->is_humongous(), "Should be no humongous regions in compaction queue");
>
>Additionally, it's confusing to see sth named "young_gc" in full-gc code (g1FullGCPrepareTask.inline.hpp).

I was too lazy to wrap it in a "movable-during-first-full-gc-attempt" method, will fix. I thought the comment would be enough.

>
>For this particular PR, could one just use !hr->is_humongous() where is_young_gc_movable is used? (Essentially, inlining >the new API.)

I do not like inlining this API because this looses the documentation of this condition. One would probably need to add a "(At the moment) Humongous regions are non-movable." comment everywhere to understand why that condition is there.

Thanks,
  Thomas

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13643#discussion_r1176691594