RFR: 8351661: NMT: VMATree should support separate call-stacks for reserve and commit operations [v28]

Johan Sjölen jsjolen at openjdk.org
Wed May 21 11:21:07 UTC 2025 

On Thu, 15 May 2025 09:31:13 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> In NMT detail mode, we need to have separate call-stacks for Reserve and Commit operations.
>> This PR adds a second stack to every node that will be used when committing (and uncommitting) the start node of a reserved region.
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
> 
>   impl of new visualization in one test case, for getting feedback.

If this passes all current tests then I'm fine with it.

Please consider all of my comments.

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

> 32: // Semantics
> 33: // This tree is used to store and track the state of virtual memory regions.
> 34: // The nodes in the tree are key-value pairs where key is the memory address and the value is the State of the memory regions.

the key

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

> 33: // This tree is used to store and track the state of virtual memory regions.
> 34: // The nodes in the tree are key-value pairs where key is the memory address and the value is the State of the memory regions.
> 35: // State of a region describes whether the region is released, reserved or committed, which MemTag it has and where in

The state (State if it's a type)

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

> 44: // changes needed for the affected regions in the tree. For each operation a request
> 45: // (<from-address, to-address, operation, tag, call-stack, which-tag-to-use >) is sent to the tree to handle.
> 46: //

``
`During these operations, the tree should handle the
// changes needed for the affected regions in the tree


I think that this can be removed without loss of information, it is clear that the tree must do this.

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

> 45: // (<from-address, to-address, operation, tag, call-stack, which-tag-to-use >) is sent to the tree to handle.
> 46: //
> 47: // The expected changes are described here for each operation:

I agree with the semantics of these operations.

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

> 60: //   - if the region is in Released state
> 61: //     - mark the region as both Reserved and Committed
> 62: //     - store the call-stack of the request to the reserve call-stack

So if `mtNone` is provided in this case, we will end up with `mtNone` as the category for this committed region? OK.

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

> 100:                            {es, es, es}    // op == Uncommit
> 101:                            };
> 102:   if (op == 2 && ex == StateType::Released) {

This is using the information that `op == 2` is 'commit', this should be well-typed.

It's better to do something like this:


const Operation op = req.op();
if (op == Operation::Commit && ex == StateType::released) {
  return rq;
}  else {
  return result[opi][state_to_index(ex)];
}


Also, just add a small comment on why we treat committed specially here.

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

> 111:   const SIndex rq = req.callstack;
> 112:   const int op = req.op_to_index();
> 113:   assert(op >= 0 && op < 4, "should be");

Add this assert to the method above as well?

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

> 138: }
> 139: 
> 140: MemTag VMATree::get_new_tag(const MemTag ex, const RequestInfo& req) const {

This code would also benefit from a `Operation RequestInfo::op()` method which separates the `use_tag_inplace` cases.

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

> 173:   //    - since we are reserving, then `a` will be added to t2. (`y` is `a`)
> 174:   //    - since we uncommitting (by reserving) then `a` is to be subtracted from t1. (`x` is `-a`).
> 175:   //    - amount of uncommitted size is in table `commit[1][4,5]` which is `-a,0` that means subtract `a` from t1.

We can rename `t1` and `t2` to `current_tag`, `operation_tag` or `change_tag`

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

> 205:   IntervalState exSt = n1->val().out; // existing state info
> 206:   assert(n1 != nullptr,"sanity");
> 207:   assert(n2 != nullptr,"sanity");

Move these up, you'll have discovered that `n1` is nullptr when dereferencing it on line 205 otherwise.

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

> 378:     }
> 379:     tty->print_cr("");
> 380:   };

Convert to UL, under `trace` and a unique log tag for the VMATree

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

> 637:   }
> 638: 
> 639:   // ************************************************************************************ Remove the 'noop' nodes that found inside the loop

Remove the asterisks.

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

> 270:   void compute_summary_diff(const SingleDiff::delta region_size, const MemTag t1, const StateType& ex, const RequestInfo& req, const MemTag new_tag, SummaryDiff& diff) const;
> 271:   void update_region(TreapNode* n1, TreapNode* n2, const RequestInfo& req, SummaryDiff& diff);
> 272:   int state_to_index(const StateType st) const {

Can't this just use the enum values? `return static_cast<int>(static_cast<uint8_t>(st));`

test/hotspot/gtest/nmt/test_vmatree.cpp line 254:

> 252:       else
> 253:         st.out.set_commit_stack(ES);
> 254:       tree.tree().upsert((VMATree::position)et.nodes[i], st);

Please always use braces in if-stmts.

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

Marked as reviewed by jsjolen (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/24028#pullrequestreview-2856676682
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099608295
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099608909
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099612695
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099618621
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099617515
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099631578
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099674188
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099691146
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099699625
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099711607
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2099785909
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2100006258
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2100013390
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2100015652

More information about the hotspot-runtime-dev mailing list