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

Thu Feb 27 18:35:29 UTC 2025

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.

This is what I found today. Will do more tomorrow...

src/hotspot/share/nmt/memReporter.cpp line 440:

> 438:     VirtualMemoryTracker::Instance::tree()->visit_committed_regions(*reserved_rgn,
> 439:                                                                   [&](CommittedMemoryRegion& committed_rgn) {
> 440:       if (committed_rgn.size() == reserved_rgn->size() && committed_rgn.call_stack()->equals(*stack)) {

If we are calling here 

`equals()`

anyhow, why not have CommittedMemoryRegion:equals() that checks both the size and the stack? This way we can simply have:

`if (committed_rgn.equals(reserved_rgn))
`

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

> 140:     if (addr != nullptr) {
> 141:       NmtVirtualMemoryLocker nvml;
> 142:       VirtualMemoryTracker::Instance::add_reserved_region((address)addr, size, stack, mem_tag);

I do not like:

`VirtualMemoryTracker::Instance::add_reserved_region`

with the `Instance` being repeated over and over.

I'd prefer `VirtualMemoryTracker::add_reserved_region` and have `Instance` be impl detail inside.

src/hotspot/share/nmt/memoryFileTracker.hpp line 32:

> 30: #include "nmt/nmtNativeCallStackStorage.hpp"
> 31: #include "nmt/vmatree.hpp"
> 32: #include "nmt/virtualMemoryTracker.hpp"

Are you 100% sure we need it here?

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

> 57:     if (_tracker == nullptr) return false;
> 58:     new (_tracker) VirtualMemoryTracker(level == NMT_detail);
> 59:     return _tracker->tree() != nullptr;

We should check for `tree() != nullptr;` inside VirtualMemoryTracker constructor as assert?

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

> 112:     committed = VirtualMemorySummary::as_snapshot()->by_type(tag)->committed();
> 113:     if (reserve_delta != 0) {
> 114:       if (reserve_delta > 0)

Missing braces.

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

> 116:       else {
> 117:         if ((size_t)-reserve_delta <= reserved)
> 118:           VirtualMemorySummary::record_released_memory(-reserve_delta, tag);

Missing braces.

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

> 127:         }
> 128:         else
> 129:           print_err("commit");

Missing braces.

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

> 131:       else {
> 132:         if ((size_t)-commit_delta <= committed)
> 133:           VirtualMemorySummary::record_uncommitted_memory(-commit_delta, tag);

Missing braces.

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

> 133:           VirtualMemorySummary::record_uncommitted_memory(-commit_delta, tag);
> 134:         else
> 135:           print_err("uncommit");

Missing braces.

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

> 211:     log_info(nmt)("region in walker vmem, base: " INTPTR_FORMAT " size: %zu , %s, committed: %zu",
> 212:      p2i(rgn.base()), rgn.size(), rgn.tag_name(), rgn.committed_size());
> 213:     if (!walker->do_allocation_site(&rgn))

Missing braces.

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

> 223: 
> 224: int compare_reserved_region_base(const ReservedMemoryRegion& r1, const ReservedMemoryRegion& r2) {
> 225:   return r1.compare(r2);

Why did we name it `compare_reserved_region_base`, not simply `compare_reserved_region`

src/hotspot/share/nmt/vmatree.cpp line 33:

> 31: const VMATree::RegionData VMATree::empty_regiondata{NativeCallStackStorage::StackIndex{}, mtNone};
> 32: 
> 33: const char* VMATree::statetype_strings[4] = {

You don't need do anything here if you take my suggestion from next comment...

src/hotspot/share/nmt/vmatree.hpp line 73:

> 71:     assert(type < StateType::COUNT, "must be");
> 72:     return statetype_strings[static_cast<uint8_t>(type)];
> 73:   }

I don't like that we are hardcoding the size of this array and have COUNT be StateType. Can we do something like this?:

  enum class StateType : uint8_t { Reserved = 1, Committed = 3, Released = 0 };

private:
  static constexpr const char* const statetype_strings[] = {"released", "reserved", "only-committed", "committed"};
  static constexpr int STATETYPE_COUNT = static_cast<int>(sizeof(statetype_strings)/sizeof(char*));

public:
  NONCOPYABLE(VMATree);

  static const char* statetype_to_string(StateType type) {
    assert(static_cast<uint8_t>(type) < STATETYPE_COUNT, "must be");
    return statetype_strings[static_cast<uint8_t>(type)];
  }

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

Changes requested by gziemski (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2648596994
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974069506
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974080149
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974082904
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974096829
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974100115
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974099677
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974100680
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974101028
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974101373
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974103095
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974105989
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974108411
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1974129696