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

[Johan Sjölen]

On Sun, 23 Feb 2025 21:50:42 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> - `VMATree` is used instead of `SortedLinkList` in new class `VirtualMemoryTrackerWithTree`.
>>  -  A wrapper/helper `RegionTree` is made around VMATree to make some calls easier.
>>  -  Both old and new versions exist in the code and can be selected via `MemTracker::set_version()`
>>  - `find_reserved_region()` is used in 4 cases, it will be removed in further PRs.
>>  - All tier1 tests pass except one ~that expects a 50% increase in committed memory but it does not happen~  https://bugs.openjdk.org/browse/JDK-8335167.
>>  - Adding a runtime flag for selecting the old or new version can be added later.
>>  - Some performance tests are added for new version, VMATree and Treap, to show the idea and should be improved later. Based on the results of comparing speed of VMATree and VMT, VMATree shows ~40x faster response time.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   once more.

Hi,

You should take out these bug fixes and add them as separate PRs instead. They need separate reviewing so that we can discuss them outside of the context of this port.

src/hotspot/share/nmt/memoryFileTracker.cpp line 183:

> 181:     // Only account the committed memory.
> 182:     snap->commit_memory(current->committed());
> 183:   });}

Style: Restore to what it was before.

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

> 402:     friend class VirtualMemoryTrackerTest;
> 403:     friend class CommittedVirtualMemoryTest;
> 404: 

These two classes doesn't exist anymore.

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

> 78:       MemTag tag = leqA_n->val().out.mem_tag();
> 79:       stA.out.set_tag(tag);
> 80:       LEQ_A.state.out.set_tag(tag);

This also seems like a bug fix that must be separated out into a separate PR along with test cases.

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

> 209:   // Finally, we can register the new region [A, B)'s summary data.
> 210:   MemTag tag_to_change = use_tag_inplace ? stA.out.mem_tag() : metadata.mem_tag;
> 211:   SingleDiff& rescom = diff.tag[NMTUtil::tag_to_index(tag_to_change)];

This seems to be a bug fix to 8335091. You should open a separate mainline PR with this fix and add a testcase for it. Your fix should be integrated before this PR is.

test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp line 1:

> 1: /*

Why are these tests removed? Can they be adapted to the new implementation?

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

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2636234435
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1967189701
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1967186209
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1967184884
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1967183983
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1967173959