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

[Afshin Zafari]

On Tue, 25 Mar 2025 10:39:52 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> Some more comments.
> 
> Can we have the example cases I presented as test cases in the code? I think that those show the behavior of this code.

Done.

> test/hotspot/gtest/nmt/test_vmatree.cpp line 764:
> 
>> 762:   auto expected =  [&](Tree& tree, int p, SIndex reserve_stack, SIndex commit_stack, int line_no = __LINE__) {
>> 763:     VMATree::VMATreap::Range r = tree.tree().find_enclosing_range(p);
>> 764:     const bool check_end_node = true;
> 
> Dead code

Removed.

> test/hotspot/gtest/nmt/test_vmatree.cpp line 782:
> 
>> 780:   };
>> 781: 
>> 782:   {
> 
> I think that each test case should at least have a small comment summarizing the supposed goal of the test. Below is an example of what the could look like for the first test case. 
> 
> 
> // Check committing into a reserved region inherits the call stacks

Done.

> test/hotspot/gtest/nmt/test_vmatree.cpp line 787:
> 
>> 785:     expected(tree,  0, si_1,   -1, __LINE__);
>> 786: 
>> 787:     tree.commit_mapping(25, 25, call_stack_2, true); // commit at the middle of the region
> 
> We have something like `[0, 25), [25, 50), [50, 100)` here, can we check the last region also?

Done.

> test/hotspot/gtest/nmt/test_vmatree.cpp line 807:
> 
>> 805: 
>> 806:     tree.commit_mapping(10, 20, call_stack_1); // commit with overlap
>> 807:     expected(tree, 10, si_1, si_1, __LINE__);
> 
> Introduce a third call stack for this case. We want to see that the reserved stack is placed correctly, it also clarifies what the test does a bit for readers.

Done.

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

PR Comment: https://git.openjdk.org/jdk/pull/24028#issuecomment-2751213108
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2012058829
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2012060453
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2012059726
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2012060806