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

[Afshin Zafari]

On Wed, 21 May 2025 08:22:06 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> 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.
>
> 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?

Done.

> 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.

Done.

> 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`

Done.

> 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.

Done.

> src/hotspot/share/nmt/vmatree.cpp line 639:
> 
>> 637:   }
>> 638: 
>> 639:   // ************************************************************************************ Remove the 'noop' nodes that found inside the loop
> 
> Remove the asterisks.

Done

> 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));`

Unfortunately not. Later in the main PR for porting VMATree to NMT, we needed to redefine `Commit` to be `3` and now it is `2`.

> 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.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101854213
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101851511
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101851150
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101850875
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101850463
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101850212
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101848047