On Tue, 25 Apr 2023 15:29:08 GMT, Thomas Schatzl <tscha...@openjdk.org> wrote:
>> 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 > Given this branch is unreachable, can one just use `fatal(...)` without a > predicate? I also thought about this: the purpose of the additional assert is/was (I think) to fail with a better message than just crashing with some nondescript message, filtering out other possible cases. Will remove this after all. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13643#discussion_r1176693623
