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

[Thomas Stuefe]

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

Started looking at Treap. More tomorrow.

About the Treap. Looking at this closer, I don't understand why you don't follow the more canonical way of implementing a treap:

- Insert: find lexical position for node x, then do tree rotations until the node priority is in line
- Delete: find node, and as long as its not a leaf, rotate until it is. Then cut node out.

Would that not be easier to understand, and faster too? No need for recursion either. We would not need merge and split at all. All we need is insert and delete, after all, we don't need any bulk operations on nodes.

Then, the Treap definitly needs an ASSERT-only verify to check consistency, and that needs to be called periodically. 

TreapNode:

Aesthetical nits: Its a mix right now. You have getters, but then the owning Treap has private access and uses that. I see you expose the TreapNode to outside access. In that case, I would prefer if you would use getters and setters consistently, and remove the friend declaration. Style-wise, the code could be more condensed.

File naming: I see we have inconsistencies. We name some files "nmtXXX", some not. All code lives inside "nmt", so the prefix could be superfluous. Just a small nit.

BTW, I won't insist on storing callstacks in a linear array. That is just a performace- and memory optimization. We can do this later in a follow up RFE.

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

PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-2030296761
PR Comment: https://git.openjdk.org/jdk/pull/18289#issuecomment-2084792287