On Tue, 25 Apr 2023 13:49:05 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 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. 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`). For this particular PR, could one just use `!hr->is_humongous()` where `is_young_gc_movable` is used? (Essentially, inlining the new API.) 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? ------------- PR Review: https://git.openjdk.org/jdk/pull/13643#pullrequestreview-1400139894 PR Review Comment: https://git.openjdk.org/jdk/pull/13643#discussion_r1176660073
