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

[Gerard Ziemski]

On Fri, 28 Feb 2025 13:55:30 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:
> 
>   style, some cleanup, VMT and regionsTree circular dep resolved

src/hotspot/share/nmt/regionsTree.cpp line 28:

> 26: VMATree::SummaryDiff RegionsTree::commit_region(address addr, size_t size, const NativeCallStack& stack) {
> 27:   return commit_mapping((VMATree::position)addr, size, make_region_data(stack, mtNone), /*use tag inplace*/ true);
> 28: }

`RegionsTree::commit_region` is called by 

 ```
 static inline void record_virtual_memory_reserve_and_commit(void* addr, size_t size,
    const NativeCallStack& stack, MemTag mem_tag = mtNone) {


which has mem_tag, so we could in theory use it and pass it down? Then we could avoid the complicated "use_tag_inplace" parameter handling?

Not sure if this is possible in all cases. Is that why we have the need for "use_tag_inplace"?

src/hotspot/share/nmt/regionsTree.cpp line 32:

> 30: VMATree::SummaryDiff RegionsTree::uncommit_region(address addr, size_t size) {
> 31:   return uncommit_mapping((VMATree::position)addr, size, make_region_data(NativeCallStack::empty_stack(), mtNone));
> 32: }

Would it be helpful here if we were to add a new tag, that would mark this uncommitted region somehow different than mtNone (to mark it that it used to be used, but now it's not, which is different from never used region)?

test/hotspot/gtest/nmt/test_regions_tree.cpp line 72:

> 70:   EXPECT_EQ(rmr.base(), (address)1400);
> 71:   rmr = rt.find_reserved_region((address)1005);
> 72:   EXPECT_EQ(rmr.base(), (address)1000);

When I do:

  rmr = rt.find_reserved_region((address)999);

I get back ReservedMemoryRegion with base == 1, I am not 100% sure what I was expecting - probably 0, but not 1.

test/hotspot/gtest/nmt/test_regions_tree.cpp line 98:

> 96:   rt.reserve_mapping(1400, 50, rd);
> 97: 
> 98:   rt.commit_region((address)1010, 5UL, ncs);

What would, what should happen if we repeat the same reserve_mapping?

rt.commit_region((address)1010, 5UL, ncs);
rt.commit_region((address)1010, 5UL, ncs);

Are/should we be allowed to do this?

test/hotspot/gtest/nmt/test_regions_tree.cpp line 102:

> 100:   rt.commit_region((address)1030, 5UL, ncs);
> 101:   rt.commit_region((address)1040, 5UL, ncs);
> 102:   ReservedMemoryRegion rmr((address)1000, 50);

I would add something like:

  rt.commit_region((address)1500, 5UL, ncs); // should not be counted

that should not be counted.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1977880730
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1977894025
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1978113833
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1978138735
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1978124450