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

[Afshin Zafari]

On Thu, 21 Nov 2024 20:39:24 GMT, Robert Toyonaga <duke at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   gc/x directory removed.
>
> src/hotspot/share/nmt/regionsTree.hpp line 33:
> 
>> 31: 
>> 32: // RegionsTree extends VMATree to add some more specific API and also defines a helper
>> 33: // for processing the tree nodes in a shorter and more menaingful way.
> 
> Typo: "meaningful"

Fixed, thanks.

> src/hotspot/share/nmt/vmtCommon.cpp line 121:
> 
>> 119:       size_t aligned_stack_size = align_up(stack_size, os::vm_page_size());
>> 120: 
>> 121:       ReservedMemoryRegion* region = const_cast<ReservedMemoryRegion*>(rgn);
> 
> Is this still needed now that you're directly adding committed regions to the tree? I think this was only there before because it was adding committed regions to the `ReservedMemoryRegion`.

Fixed.

> src/hotspot/share/nmt/vmtCommon.cpp line 133:
> 
>> 131:           committed_size = stack_bottom + stack_size - committed_start;
>> 132:         }
>> 133:         VirtualMemoryTracker::Instance::add_committed_region(committed_start, committed_size, ncs);
> 
> Is it a problem that it is never recorded that regions are uncommitted?  Committed regions are added on-the-fly when reporting data, and those regions persist after reporting is done. So maybe it's possible to report too much committed size on a future report. The old implementation had this characteristic too.

Uncommit for mtThreadStack are tracked when a thread exits and cleans/releases its stack.
Here, the committed pages that are not reported yet to NMT are extracted.

> src/hotspot/share/runtime/os.cpp line 2213:
> 
>> 2211:     res = pd_release_memory(addr, bytes);
>> 2212:     if (res) {
>> 2213:       ThreadCritical tc;
> 
> What is the reason for moving `pd_release_memory` out of the protection of the lock? Wouldn't it be better if the memory operation and the accounting were done together atomically like with [`os::uncommit_memory`](https://github.com/openjdk/jdk/pull/20425/files#diff-c626d392182b0193ad0e754de3f4fd42c98d959edade67490b6e1f24be38efdbR2189) for example.

TBH, I don't know and this is not a change in this PR. The uncommit_memory has the same approach. I just know that if we move the pd_xxxx inside the lock some tests fail or some assertions happen, but do not remember which and why.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1856515803
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1856517353
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1856515481
PR Review Comment: https://git.openjdk.org/jdk/pull/20425#discussion_r1856511664