RFR: 8312132: Add tracking of multiple address spaces in NMT [v54]

[Gerard Ziemski]

On Mon, 29 Apr 2024 12:32:39 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> This PR introduces a new abstraction to NMT, named `MemoryFileTracker`. Today, NMT does not track any memory outside of the virtual memory address space. This means that if you allocated memory in something such as a memory-backed file and use `mmap` to map into that memory, then you'll have trouble reporting this to NMT. This is the situation that ZGC is in, and that is what this patch attempts to fix.
>> 
>> ## `MemoryFileTracker`
>> 
>> The `MemoryFileTracker` adds the ability of adding new virtual memory address spaces to NMT and committing memory to these, the basic API is:
>> 
>> ```c++
>>     static MemoryFile* make_device(const char* descriptive_name);
>>     static void free_device(MemoryFile* device);
>> 
>>     static void allocate_memory(MemoryFile* device, size_t offset, size_t size,
>>                                 MEMFLAGS flag, const NativeCallStack& stack);
>>     static void free_memory(MemoryFile* device, size_t offset, size_t size);
>> 
>> 
>> It is easiest to see how this is used by looking at what ZGC's `ZNMT` class does:
>> 
>> ```c++
>> void ZNMT::reserve(zaddress_unsafe start, size_t size) {
>>   MemTracker::record_virtual_memory_reserve((address)start, size, CALLER_PC, mtJavaHeap);
>> }
>> void ZNMT::commit(zoffset offset, size_t size) {
>>   MemTracker::allocate_memory_in(ZNMT::_device, static_cast<size_t>(offset), size, mtJavaHeap, CALLER_PC);
>> }
>> void ZNMT::uncommit(zoffset offset, size_t size) {
>>   MemTracker::free_memory_in(ZNMT::_device, (size_t)offset, size);
>> }
>> 
>> void ZNMT::map(zaddress_unsafe addr, size_t size, zoffset offset) {
>>   // NMT doesn't track mappings at the moment.
>> }
>> void ZNMT::unmap(zaddress_unsafe addr, size_t size) {
>>   // NMT doesn't track mappings at the moment.
>> }
>> 
>> 
>> As you can see, any mapping between reserved regions and device-allocated memory is not recorded in NMT. This means that in detailed mode you only get reserved regions printed for the reserved memory, the device-allocated memory is reported separately. When performing summary reporting any memory allocated via these devices is added to the corresponding `MEMFLAGS` as `committed` memory. 
>> 
>> This patch is also acting as a base on which we deploy multiple new backend ideas to NMT. These ideas are:
>> 
>> 1. Implement VMA tracking using a balanced binary tree approach. Today's `VirtualMemoryTracker`'s usage of linked lists is slow and brittle, we'd like to move away from it. Our Treap-based approach in this patch gives a performance bo...
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   assert device != nullptr in MemoryFileTracker::instance

Still working on it and have more to go over, but here is the feedback for now.

src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 53:

> 51:     summary->reserve_memory(diff.flag[i].reserve);
> 52:   }
> 53: }

I'm probably missing something here:

Why do we need to mark the reservation for all nmt flag types, when the API takes a single flag as argument here?

If later I was to iterate over all flags, all summaries would show the same reservation? Isn't that simply wrong?

src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 52:

> 50:   };
> 51:   NativeCallStack* put(const NativeCallStack& value) {
> 52:     int bucket = value.calculate_hash() % nr_buckets;

`calculate_hash()` is:


    for (int i = 0; i < NMT_TrackingStackDepth; i++) {
      hash += (uintptr_t)_stack[i];
    }


Wouldn't XOR serve us better here than plain "+" ?

src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 67:

> 65:   // 4096 buckets ensures that probability of collision is 50% at approximately 64
> 66:   // different call stacks.
> 67:   static const constexpr int nr_buckets = 4096;

Shouldn't that be a prime number optimally, ex. 4099? (ideally Marsenne prime, but there is one at 127 then next one is 8191)

src/hotspot/share/nmt/vmatree.cpp line 29:

> 27: #include "utilities/growableArray.hpp"
> 28: 
> 29: VMATree::SummaryDiff VMATree::register_mapping(size_t A, size_t B, StateType state,

Does `VMATree` stand for "Virtual Memory Allocation Tree"?

We have some long name already in nmt, ex: `NativeCallStackStorage`, `MemoryFileTracker`, `MallocSiteHashtableEntry`, `MemSummaryDiffReporter`. Can we then name it: `VirtualMemAllocationTree` or `VirtualMemAllocTree` or `VirtMemAllocTree` ?

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

PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-2028962210
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1583719980
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1583356517
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1583385203
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1583635036