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

[Thomas Schatzl]

On Thu, 27 Apr 2023 10:38:17 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

> > 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) &&
> ```
> 

`G1CollectionSetChooser::should_add` asks: can/should I add this region to the collection set candidates to evacuate (reclaim via moving) this region during young gc?

> 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()) {
> ```

`G1FullCollector::before_marking_update_attribute_table` asks: can I compact/move this region in the (small object) compaction phase later?

So they are asking the question for different types of gc, where in the second case it is actually asking that question for a phase that is about compacting regular object regions. So it seems somewhat obvious to exclude non-regular object regions at the outset, or at least not use this predicate (which you criticized as non-obvious why full gc uses a predicate with "young-gc" inside).

Then there is the matter of documentation: if one writes `!is_humongous()` there, there is need for a comment like "we do not move humongous objects during young gc" everywhere you need it, while the method name also acts as the documentation, saying "exclude everything that we are not moving during young gc".

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

Combining the two into the single predicate may be correct from an execution POV. However the two predicates filter for different reasons: The `!is_young` filters out regions that are not allowed to be put in the collection set candidates at all (it's a set of old regions that young gc may evacuate later by definition), the second filters those that can't be reclaimed by moving/evacuation.

Otherwise one would need to add comments, this way it is self-commenting (and this isn't performance sensitive).

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

`is_young_gc_movable` and pinning are separate concerns. `is_young_gc_movable` is a static view on the region. Pinning is assumed to be very transient, and assumed to not pin too much (generating lots of garbage in pinned regions basically - everything but the potentially pinned objects are still evacuated out).
So it is more than likely advantageous to put pinned regions into the candidates for proactive evacuation.

Thanks,
  Thomas

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

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