On Thu, 27 Apr 2023 10:38:17 GMT, Albert Mingkun Yang <[email protected]> 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
