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

Thu May 9 11:39:03 UTC 2024

On Wed, 8 May 2024 11:53:17 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:
> 
>   Some style

More treap comments. Will continue later.

src/hotspot/share/nmt/nmtTreap.hpp line 203:

> 201:     TreapNode* last_seen = nullptr;
> 202:     bool failed = false;
> 203:     this->visit_in_order([&](TreapNode* node) {

Here, and in other places: what's with the this-> ?

src/hotspot/share/nmt/nmtTreap.hpp line 209:

> 207:       }
> 208:       int c = COMPARATOR::cmp(last_seen->key(), node->key());
> 209:       if (c > 0) {

No need for c, combine those lines

src/hotspot/share/nmt/nmtTreap.hpp line 230:

> 228: 
> 229:   void upsert(const K& k, const V& v) {
> 230:     assert(verify_self(), "invariant");

Here, and down in remove(): I would consider modifying the verification here, or even remove it, and call it explicitly by callers of the treap at convenient times only. My experiences from doing very similar things in Metaspace was that such fine grained verifications get quickly way to expensive, even for a debug build.

In metaspace, I ended up doing something like this: https://github.com/openjdk/jdk/blob/ac86f59e4f5382d5c3e8984532dd210611db7dcb/src/hotspot/share/memory/metaspace/metaspaceCommon.hpp#L119

which adds assertion that are only checked every n times, in that case configurable with a switch.

src/hotspot/share/nmt/nmtTreap.hpp line 240:

> 238:     DEBUG_ONLY(_node_count++;)
> 239:     // Doesn't exist, make node
> 240:     void* node_place = ALLOCATOR::allocate(sizeof(TreapNode));

Please make it explicit in the class definition that the ALLOCATOR must be checking for oom and exit or whatever.

src/hotspot/share/nmt/nmtTreap.hpp line 254:

> 252: 
> 253:     // (LEQ_k, GT_k)
> 254:     node_pair fst_split = split(this->_root, k, LEQ);

Can we afford some more letters please? :-) fst : first snd : second
But since you use left and right in other places, I'd use that too here.

src/hotspot/share/nmt/nmtTreap.hpp line 278:

> 276:       to_delete.push(head->_left);
> 277:       to_delete.push(head->_right);
> 278:       ALLOCATOR::free(head);

If we ever generalize this treap, we may want to add an optional dtor call for the payload here. Not for now, though

src/hotspot/share/nmt/nmtTreap.hpp line 287:

> 285:     if (leqB != nullptr && leqB->key() == key) {
> 286:       return leqB;
> 287:     }

I don't get this, why is this leq search needed? And if its needed, is that not redundant to the code below that compares for key equality?

src/hotspot/share/nmt/nmtTreap.hpp line 307:

> 305:   }
> 306: 
> 307:   TreapNode* closest_leq(const K& key) {

I don't understand the naming of the variables. What is A? _n? _r? 
And "_head" is somewhat misleading. I would have named head=pos or current, leqA_n = best or found or candidate or best_so_far... any of these

src/hotspot/share/nmt/nmtTreap.hpp line 321:

> 319:         head = head->_right;
> 320:       } else if (cmp_r > 0) {
> 321:         head = head->_left;

Just use else, and optionally assert != 0

test/hotspot/gtest/nmt/test_nmt_treap.cpp line 57:

> 55:       treap.remove(i);
> 56:     }
> 57:   }

Please test the find functions (leq, geq etc) and iteration.

For the latter, please test that we see every value, guaranteed, and that we see every value only exactly once.

Also test:
- inserting duplicates should result in only one value
- empty treap should work as expected
- treap with custom allocator, e.g. a simple array based one. Make sure we don't leak memory after nodes are removed, or after remove_all
- treap with different custom comparator, please counter check the iteration order
- maybe some more types? at least the common numericals

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

PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-2043200272
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595280793
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595281186
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595327894
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595287004
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595289941
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595291696
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595305355
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595296855
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595294771
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1595321380