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

[Afshin Zafari]

On Wed, 21 May 2025 07:50:18 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 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

Done.

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

Done.

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

Done. Not sure as you wanted. 
Please double check!

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

Hmm, what should I do here?

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

True. What is the proper thing to do for this case?

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

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101866848
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101866548
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101866295
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101862999
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101865193
PR Review Comment: https://git.openjdk.org/jdk/pull/24028#discussion_r2101861180