RFR: 8337563: NMT: rename MEMFLAGS to MemTag

[David Holmes]

On Thu, 5 Sep 2024 16:10:05 GMT, Gerard Ziemski <gziemski at 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