RFR: 8337217: Port VirtualMemoryTracker to use VMATree [v32]

[Johan Sjölen]

On Thu, 27 Feb 2025 10:37:33 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> - `VMATree` is used instead of `SortedLinkList` in new class `VirtualMemoryTracker`.
>>  -  A wrapper/helper `RegionTree` is made around VMATree to make some calls easier.
>>  - `find_reserved_region()` is used in 4 cases, it will be removed in further PRs.
>>  - All tier1 tests pass except this https://bugs.openjdk.org/browse/JDK-8335167.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   removed UseFlagInPlace test.

More review comments.

src/hotspot/share/nmt/memTracker.hpp line 60:

> 58:   static bool walk_virtual_memory(VirtualMemoryWalker* walker) {
> 59:     return VirtualMemoryTracker::Instance::walk_virtual_memory(walker);
> 60:   }

The `MemTracker` API exposes the outer and locking implementation to the rest of Hotspot. These two methods are used by us internally. I think it's better if these methods are deleted and the `VirtualMemoryTracker::Instance` methods are called directly, instead.

src/hotspot/share/nmt/nmtTreap.hpp line 388:

> 386:       head = to_visit.pop();
> 387:       if (!f(head))
> 388:         return;

Style: Always use braces in `if` statements.

src/hotspot/share/nmt/nmtTreap.hpp line 416:

> 414:       if (cmp_from >= 0 && cmp_to < 0) {
> 415:         if (!f(head))
> 416:           return;

Style: Always use braces in if statements.

src/hotspot/share/nmt/regionsTree.hpp line 30:

> 28: #include "nmt/nmtCommon.hpp"
> 29: #include "nmt/vmatree.hpp"
> 30: #include "nmt/virtualMemoryTracker.hpp"

This doesn't seem used. However, you do not include the `nmt/nmtNativeCallStackStorage.hpp` header.

src/hotspot/share/nmt/regionsTree.hpp line 49:

> 47:   using Node = VMATree::TreapNode;
> 48: 
> 49:   class NodeHelper {

Most of the methods here should be `const` and take `const NodeHelper&` as arguments.

src/hotspot/share/nmt/regionsTree.hpp line 62:

> 60:       inline VMATree::StateType in_state() { return _node->val().in.type(); }
> 61:       inline VMATree::StateType out_state() { return _node->val().out.type(); }
> 62:       inline size_t distance_from(NodeHelper& other) { return position() - other.position(); }

`assert(position() > other.position()`.

src/hotspot/share/nmt/regionsTree.hpp line 82:

> 80:                      );
> 81:       }
> 82:     };

1. Doesn't need to be inline, move to `cpp` file.

2. No need to cast to `int`, just use the `VMATree::StateType` directly.

3. Should be wrapped in `#ifdef ASSERT` probably, I don't see us shipping this in product builds.

src/hotspot/share/nmt/regionsTree.hpp line 90:

> 88:       return true;
> 89:     });
> 90:   }

Move to `cpp` file, wrap in `#ifdef ASSERT`.

src/hotspot/share/nmt/virtualMemoryTracker.cpp line 61:

> 59:     return _tracker->tree() != nullptr;
> 60:   }
> 61:   return true;

```c++
void* tracker = os::malloc(sizeof(VirtualMemoryTracker), mtNMT),
if (tracker == nullptr) return false;
_tracker = new (tracker) VirtualMemoryTracker(level == NMT_detail);

src/hotspot/share/nmt/virtualMemoryTracker.cpp line 105:

> 103:     //                " vms-committed: %zu",
> 104:     //                str, NMTUtil::tag_to_name(tag), (long)reserve_delta, (long)commit_delta, reserved, committed);
> 105:   };

Any plan regarding this?

src/hotspot/share/nmt/virtualMemoryTracker.cpp line 319:

> 317:         }
> 318:         VirtualMemoryTracker::Instance::add_committed_region(committed_start, committed_size, ncs);
> 319:         //log_warning(cds)("st start: " INTPTR_FORMAT " size: " SIZE_FORMAT, p2i(committed_start), committed_size);

Outdated log

src/hotspot/share/nmt/virtualMemoryTracker.hpp line 300:

> 298: 
> 299:  public:
> 300:   CommittedMemoryRegion() :

Style:
```c==
CommittedMemoryRegion()
  : VirtualMemoryRegion((address)1, 1), _stack(NativeCallStack::empty_stack()) { }

src/hotspot/share/nmt/virtualMemoryTracker.hpp line 322:

> 320:   bool is_valid() { return base() != (address)1 && size() != 1;}
> 321:   ReservedMemoryRegion() :
> 322:     VirtualMemoryRegion((address)1, 1), _stack(NativeCallStack::empty_stack()), _mem_tag(mtNone) { }

Style: Space between `is_valid` and constructor, fix initializer list as in `CommittedMemoryRegion`.

src/hotspot/share/nmt/virtualMemoryTracker.hpp line 372:

> 370: class VirtualMemoryTracker {
> 371:  private:
> 372:   RegionsTree *_tree;

`class RegionsTree;` shouldn't be needed if you fix the circular include from above. There is no need to have the `private:` specifier. The `_tree` doesn't need to be a pointer after the forward declaration is removed, which in turn simplifies the initialization code.

test/hotspot/gtest/nmt/test_nmt_treap.cpp line 30:

> 28: #include "runtime/os.hpp"
> 29: #include "unittest.hpp"
> 30: #include "utilities/linkedlist.hpp"

Outdated header inclusion

-------------

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2647773937
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973578352
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973584937
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973585248
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973610404
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973594825
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973594257
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973588473
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973589270
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973604346
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973603631
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973601861
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973599416
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973600519
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973612354
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1973613179