On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski <gziem...@openjdk.org> wrote:
> Please review this cleanup, where we rename `MEMFLAGS` to `MemTag`. > > `MEMFLAGS` implies that we can use more than one at the same time, but those > are exclusive values, so `MemTag` is a more suitable name. > > This fix also includes a cleanup of all the related parameter names and local > variable names. > > Testing is pending... > > Note: there is more history in old closed PRs > [https://github.com/openjdk/jdk/pull/20497](https://github.com/openjdk/jdk/pull/20497) > and > [https://github.com/openjdk/jdk/pull/20472](https://github.com/openjdk/jdk/pull/20472) The bulk of this seems fine but as Kim points out the `F` template parameter really needs changing too. There are also some other questionable changes that I've flagged below. Thanks src/hotspot/share/nmt/memMapPrinter.cpp line 70: > 68: //end > 69: > 70: static const char* get_shortname_for_nmt_flag(MemTag mem_tag) { Shouldn't this be renamed to `get_shortname_for_nmt_tag`? src/hotspot/share/nmt/memReporter.cpp line 852: > 850: } else if (early_site->mem_tag() != current_site->mem_tag()) { > 851: // This site was originally allocated with one type, then > released, > 852: // then re-allocated at the same site (as far as we can tell) > with a different type. s/type/tag/ src/hotspot/share/nmt/memTracker.hpp line 83: > 81: if (enabled()) { > 82: return MallocTracker::record_malloc(mem_base, size, mem_tag, stack); > 83: return MallocTracker::record_malloc(mem_base, size, mem_tag, stack); Did this even compile? ! Suggestion: return MallocTracker::record_malloc(mem_base, size, mem_tag, stack); src/hotspot/share/nmt/memoryFileTracker.cpp line 51: > 49: for (int i = 0; i < mt_number_of_tags; i++) { > 50: VirtualMemory* summary = > file->_summary.by_type(NMTUtil::index_to_tag(i)); > 51: summary->reserve_memory(diff.type[i].commit); Why is this `type` not `tag`? src/hotspot/share/nmt/memoryFileTracker.cpp line 109: > 107: tty->print_cr("Expected start out to have same type as end in, but > was: %s, %s", > 108: > VMATree::statetype_to_string(broken_start->val().out.state()), > 109: > VMATree::statetype_to_string(broken_end->val().in.state())); Not seeing what this rename has to do with current changes. ??? src/hotspot/share/nmt/virtualMemoryTracker.cpp line 400: > 398: > 399: // Print some more details. Don't use UL here to avoid > circularities. > 400: tty->print_cr("Error: existing region: [" INTPTR_FORMAT "-" > INTPTR_FORMAT "), type %u.\n" Again why `type` instead of `tag`? src/hotspot/share/nmt/virtualMemoryTracker.cpp line 560: > 558: // Given an existing memory mapping registered with NMT, split the > mapping in > 559: // two. The newly created two mappings will be registered under the call > 560: // stack and the memory types of the original section. types -> tags src/hotspot/share/nmt/vmatree.cpp line 86: > 84: // If the state is not matching then we have different > operations, such as: > 85: // reserve [x1, A); ... commit [A, x2); or > 86: // reserve [x1, A), type1; ... reserve [A, x2), type2; or Why type not tag? src/hotspot/share/nmt/vmatree.hpp line 91: > 89: private: > 90: // Store the state and mem_tag as two bytes > 91: uint8_t info[2]; I'm unclear about terminology here: type -> state ? ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20872#pullrequestreview-2288637165 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749408498 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749409032 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749409609 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749410180 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749410493 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749411164 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749411446 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749411720 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1749412298
