On Wed, 26 Apr 2023 09:12:24 GMT, Thomas Schatzl <tscha...@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
