On Fri, 26 Jun 2026 09:51:24 GMT, Ivan Walulya <[email protected]> wrote:
>> Hi, >> >> Please review this change to rename G1 region count fields to use >> num_regions or num_*_regions consistently where the value represents a >> number regions. Keep length terminology where the collection explicitly >> refers to the length of a list. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ivan Walulya has updated the pull request incrementally with one additional > commit since the last revision: > > StefanK review Changes requested by tschatzl (Reviewer). src/hotspot/share/gc/g1/g1AllocRegion.inline.hpp line 34: > 32: #define assert_alloc_region(p, message) > \ > 33: do { > \ > 34: assert((p), "[%s] %s c: %u r: " PTR_FORMAT, > \ I think the `c:` meant "count". src/hotspot/share/gc/g1/g1HeapRegionSet.cpp line 253: > 251: > 252: verify_optional(); > 253: DEBUG_ONLY(uint old_length = num_regions();) old_length -> num_old_regions? src/hotspot/share/gc/g1/g1HeapRegionSet.cpp line 263: > 261: > 262: G1HeapRegion* curr = first; > 263: uint removed = 0; `num_removed` maybe? (Or num_regions_removed) src/hotspot/share/gc/g1/g1HeapRegionSet.cpp line 300: > 298: assert(num_regions() + num_regions_to_remove == old_length, > 299: "[%s] new length should be consistent " > 300: "new length: %u old length: %u num_regions: %u", length -> old_num_regions etc.? src/hotspot/share/gc/g1/g1HeapRegionSet.hpp line 33: > 31: #define assert_heap_region_set(p, message) \ > 32: do { \ > 33: assert((p), "[%s] %s ln: %u", \ I believe the `ln` meant `length`, so needs to be renamed too. Maybe spell it out completely (num_regions). Same in the other asserts. src/hotspot/share/gc/g1/g1HeapRegionSet.hpp line 64: > 62: // Base class for all the classes that represent heap region sets. It > 63: // contains the basic attributes that each set needs to maintain > 64: // (e.g., length, region num, used bytes sum) plus any shared "length" src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java line 253: > 251: G1HeapRegionSetBase oldSet = g1h.oldSet(); > 252: G1HeapRegionSetBase humongousSet = g1h.humongousSet(); > 253: long oldGenRegionNum = oldSet.numRegions() + > humongousSet.numRegions(); Maybe these locals ending in `Num` could be fixed up as well. ------------- PR Review: https://git.openjdk.org/jdk/pull/31688#pullrequestreview-4578655458 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480924997 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480701433 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480702475 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480914800 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480909080 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480920677 PR Review Comment: https://git.openjdk.org/jdk/pull/31688#discussion_r3480932199
