On Mon, 9 Sep 2024 00:13:25 GMT, David Holmes <dhol...@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) > > 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`? I went with `get_shortname_for_mem_tag()` I think that is more consistent? Is that OK? > 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/ Fixed. > 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); Fixed. None of the compilers had a problem with this weirdly enough. > 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`? Fixed. > 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. ??? Fixed. > 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`? Fixed. > 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 Fixed. > 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? I will fix it. > 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 ? Those come from my initial fix, when I renamed `MEMFLAGS` -> `MemType`. In that work the type_flag was clashing with mem_type parameter, so I renamed it from `type` to `state` I tried to re-use that original patch, but as you just found, some of those changes do not apply. I will undo this change. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750598453 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750596573 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750595789 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750592818 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750589625 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750588916 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750587807 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750586650 PR Review Comment: https://git.openjdk.org/jdk/pull/20872#discussion_r1750583730
