RFR: 8337563: NMT: rename MEMFLAGS to MemTag

[Gerard Ziemski]

On Mon, 9 Sep 2024 00:13:25 GMT, David Holmes <dholmes 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)
>
> 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