On Tue, 25 Apr 2023 15:05:10 GMT, Albert Mingkun Yang <ay...@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 > > src/hotspot/share/gc/g1/g1HeapVerifier.cpp line 414: > >> 412: // There are no other valid region types. Check for one invalid >> 413: // one we can identify before crashing: non-movable. >> 414: assert(hr->is_young_gc_movable(), "Heap region %u is >> non-movable.", hr->hrm_index()); > > Given this branch is unreachable, can one just use `fatal(...)` without a > predicate? >The name, is_young_gc_movable, seems to suggest it is context sensitive. Maybe >it's clearer if it's not defined as an API of heap-region, like other >region-type-predicates. It is defined in `G1Policy`, there is simply a shortcut via `HeapRegion`. I considered removing that shortcut in `HeapRegion`, but then this would mean lots of clutter everywhere to get to the policy object. I will find a better location for a helper. > >For example, it's odd to see them on the same abstraction level, when the two >APIs refer to sth quite different. > > assert(hr->is_young_gc_movable(), "Should only be movable region in > compaction queue"); > assert(!hr->is_humongous(), "Should be no humongous regions in compaction > queue"); > >Additionally, it's confusing to see sth named "young_gc" in full-gc code >(g1FullGCPrepareTask.inline.hpp). I was too lazy to wrap it in a "movable-during-first-full-gc-attempt" method, will fix. I thought the comment would be enough. > >For this particular PR, could one just use !hr->is_humongous() where >is_young_gc_movable is used? (Essentially, inlining >the new API.) I do not like inlining this API because this looses the documentation of this condition. One would probably need to add a "(At the moment) Humongous regions are non-movable." comment everywhere to understand why that condition is there. Thanks, Thomas ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13643#discussion_r1176691594
