RFR: 8306836: Remove pinned tag for G1 heap regions [v3]

[Albert Mingkun Yang]

On Wed, 26 Apr 2023 09:12:24 GMT, Thomas Schatzl <tschatzl 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
>
> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cplummer review

> I think you are right about using is_humongous() directly here: the reason we skip compacting of humongous regions during the "main" compaction is intentional here

However, I am unable to discern the difference -- why `is_young_gc_movable` is semantically-correct in one place, but not in the other in this concrete example.


bool G1CollectionSetChooser::should_add(HeapRegion* hr) {
  return !hr->is_young() &&
         G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr) &&


vs


void G1FullCollector::before_marking_update_attribute_table(HeapRegion* hr) {
  if (hr->is_free()) {
    _region_attr_table.set_free(hr->hrm_index());
  } else if (hr->is_humongous()) {


Looking at where `G1CollectionSetChooser::should_add` is called, can one use `hr->is_old()` instead of `!hr->is_young() && G1CollectedHeap::heap()->policy()->is_young_gc_movable(hr)`? (In fact, I believe that inlining `should_add` to the caller would result in a smoother code flow and prevent some duplicate region-type queries.)

In my opinion, introducing a new `is_young_gc_movable` API in this particular PR may not be entirely justified. It may make more sense to introduce it in later PRs where region-pinning is supported and the API is actually utilized.

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

PR Comment: https://git.openjdk.org/jdk/pull/13643#issuecomment-1525434952