RFR: 8306836: Remove pinned tag for G1 heap regions

[Thomas Schatzl]

On Tue, 25 Apr 2023 15:29:08 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

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

> Given this branch is unreachable, can one just use `fatal(...)` without a predicate?

I also thought about this: the purpose of the additional assert is/was (I think) to fail with a better message than just crashing with some nondescript message, filtering out other possible cases. 
Will remove this after all.

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

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