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

[Robert Toyonaga]

On Mon, 2 Dec 2024 15:27:39 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:
> 
>   a missed change in a shenandoah file.

Hi @afshin-zafari I left a few more comments and questions. Thanks!

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

> 55:       inline void clear_node() { _node = nullptr; }
> 56:       inline VMATree::position position() { return _node->key(); }
> 57:       inline bool is_committed_begin() { return ((uint8_t)out_state() & (uint8_t)VMATree::StateType::Committed) >= 2; }

This allows a region to be committed but not reserved?

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

> 92:   template<typename F>
> 93:   void visit_committed_regions(position start, size_t size, F func) {
> 94:     size_t end = start + size + 1;

> Yes it is missed.
In the visit_range_in_order code, the variable cmp_to is never checked against 0 (cmp_to == 0).

You are referring to [here](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/nmt/nmtTreap.hpp#L380) right? I think that is actually ok without the +1. The upper boundary, "end", is outside of the range of the region, so probably should not be checked. For example if the region's starting address is 0 and has a size of 10, the last address within the region is 9. "End" in this case is 10 (0+10), and is out of bounds. If we made "end" = 0 + 10 + 1 = 11, then we would be including 10 in the checked range, which isn't right.

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

> 103:         if (prev.is_committed_begin()) {
> 104:           comm_size += curr.distance_from(prev);
> 105:           if (!curr.is_committed_begin()) {

Shouldn't it be impossible to have 2 back-to-back committed regions? Wouldn't they already be coalesced by `VMATree::register_mapping`?

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

> 133:       }
> 134:       prev = curr;
> 135:       if (curr.is_released_begin() || begin_node.out_tag() != curr.out_tag()) {

It looks like you are running `func(ReservedMemoryRegion)` on partial reserved memory regions, so long as `begin_node.out_tag() != curr.out_tag()`. For example you may run `func(ReservedMemoryRegion)` on a slice of a reserved region, or a single committed region. Isn't the intention to instead find where each whole reserved region starts and ends, then run  `func(ReservedMemoryRegion)` on the entire reserved region?

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

> 135:       if (curr.is_released_begin() || begin_node.out_tag() != curr.out_tag()) {
> 136:         auto st = stack(curr);
> 137:         size_t r_size = curr.distance_from(begin_node);

Why is `r_size` never used?

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

PR Review: https://git.openjdk.org/jdk/pull/20425#pullrequestreview-2483028458
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1872189753
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1873544134
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1873390207
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1873539712
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1873510959