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

Thu May 9 11:39:06 UTC 2024

On Tue, 7 May 2024 11:56:33 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 four additional commits since the last revision:
> 
>  - Remove GEQ_B
>  - Move things around slightly to be closer to usage
>  - Simplify code
>  - Remove superfluous comment

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

> 64:         _right(nullptr) {
> 65:     }
> 66: 

condense this code a little? Getters can be one-liners

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

> 85:   TreapNode* _root;
> 86:   uint64_t _prng_seed;
> 87:   DEBUG_ONLY(int _node_count;)

I would enable _node_count for release, too. We don't save much by omitting, and we should print the tree state - with at least the node count - out as part of NMT diagnostics and in the NMT section of the hs-err file.

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

> 92:     static const uint64_t PrngAdd = 0xB;
> 93:     static const uint64_t PrngModPower = 48;
> 94:     static const uint64_t PrngModMask = (static_cast<uint64_t>(1) << PrngModPower) - 1;

can all be constexpr

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

> 168:   }
> 169: 
> 170:   bool verify_self() {

I would not return false, and then assert in the parent. I would assert right in here. Terser code, and you can write much clearer assertion messages without having to think about passing error infos up to the caller.

If you are worried about gtests blowing up instead of giving errors, I would not care. If this tree implementation has a bug, we need to fix it immediately anyway.

(Note that I follow that pattern - asserting verifiers called also in gtests - for a lot of code)

I would also make this method debug-only.

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

> 169: 
> 170:   bool verify_self() {
> 171:     double expected_maximum_depth = log(this->_node_count+1) * 5;

Here, and positive_infinity: would make those const or constexpr, whatever applies

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

> 187:       if (maximum_depth_found < head.depth) {
> 188:         maximum_depth_found = head.depth;
> 189:       }

can be a one liner with MAX2

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

> 197:       return false;
> 198:     }
> 199:     // Visit everything in order, see that the key ordering is monotonically increasing.

I also would verify the node count here.

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

> 219:   _prng_seed(seed)
> 220:   DEBUG_ONLY(COMMA _node_count(0)) {
> 221:   }

weird indentation

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592502963
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592677155
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592677689
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592491901
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592498423
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592488033
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592511141
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1592659499