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

[Gerard Ziemski]

On Tue, 4 Mar 2025 09:25:26 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> src/hotspot/share/nmt/regionsTree.cpp line 28:
>> 
>>> 26: VMATree::SummaryDiff RegionsTree::commit_region(address addr, size_t size, const NativeCallStack& stack) {
>>> 27:   return commit_mapping((VMATree::position)addr, size, make_region_data(stack, mtNone), /*use tag inplace*/ true);
>>> 28: }
>> 
>> `RegionsTree::commit_region` is called by 
>> 
>>  ```
>>  static inline void record_virtual_memory_reserve_and_commit(void* addr, size_t size,
>>     const NativeCallStack& stack, MemTag mem_tag = mtNone) {
>> 
>> 
>> which has mem_tag, so we could in theory use it and pass it down? Then we could avoid the complicated "use_tag_inplace" parameter handling?
>> 
>> Not sure if this is possible in all cases. Is that why we have the need for "use_tag_inplace"?
>
> I have to separate the concerns as follow:
> 1. In `record_..._reserve_and_commit` the `mem_tag` is available since it is needed for `reserve`. So, you are right, we can use the tag and pass it down. The change set would be:
>  `VMT::Instance::add_committed_region(addr, size, stack, MemTag = mtNone)`
>  `VMT::add_committed_region(addr, size, stack, MemTag = mtNone)`
>  `RegionsTree::commit_region(addr, size, stack, MemTag = mtNone)`
> 3. Even if we do so, `use_tag_in_place` is needed in `commit` because the `os::commit_memmory` family do not pass mem_tag down. There is already an abandoned PR which tried to add MemTag param to this family. It was reviewed as not necessary.
> 4. In addition, the `VMATree::register_mapping` has a mandatory MemTag param (in its MetaData param) which enforces the caller to pass down an specific MemTag. If we don't use `use_tag_inplace`, for every commit of region $[ base, end)$ we have to find the enclosing reserved region $[ A, B)$ where $A \le base < end \le B$ and get its mem_tag and then pass it down to `VMATree::register_mapping`. Finding the enclosing region could be too expensive and is preferred to be avoided. 
> 
> I can implement the case 1 above if it is preferred.

It would help with cleaning mem_tags issue, where we are trying to clean up "mtNone" tags.

>> src/hotspot/share/nmt/regionsTree.cpp line 32:
>> 
>>> 30: VMATree::SummaryDiff RegionsTree::uncommit_region(address addr, size_t size) {
>>> 31:   return uncommit_mapping((VMATree::position)addr, size, make_region_data(NativeCallStack::empty_stack(), mtNone));
>>> 32: }
>> 
>> Would it be helpful here if we were to add a new tag, that would mark this uncommitted region somehow different than mtNone (to mark it that it used to be used, but now it's not, which is different from never used region)?
>
> An uncommitted region has implicitly the same tag as its containing reserved region:
> 
> <-------------Reserved Region, mtXXX---------->
> <----C1----><..U1...><---C2--><..U2..><---C3-->
> 
> C1-C3 are committed regions with tag `mtXXX`
> U1 and U2 are uncommitted and enclosed by a `mtXXX` tag region.
> 
> Do you have any specific use-case for this?
> 
> ---
> If you were thinking about *release_mapping* instead, then I can say that the released region will be sooner or later reserved by another memtag. For example, an mtStack region is released and immediately reserved by mtGC. In other words, any memtag on a released region would be overwritten by other uses of the same region.

I was thinking in terms of cleaning up all the mtNone tags. Here we could set it to something more meaningful than mtNone?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1979749939
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1979752152