[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8306836: Remove pinned tag for G1 heap regions [v3]
From:       Thomas Schatzl <tschatzl () openjdk ! org>
Date:       2023-04-27 13:25:53
Message-ID: EHBr9TffozIoBzjWSsW9wZvbl_sLKrMkMi5HKQA7eMU=.6ea074c9-2ac0-43ef-bb66-b21607f491ad () github ! com
[Download RAW message or body]

On Thu, 27 Apr 2023 10:38:17 GMT, Albert Mingkun Yang <ayang@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


[prev in list] [next in list] [prev in thread] [next in thread]