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

Fri May 10 12:15:25 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 reviewing, started with VMATree

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

> 371:     }
> 372:   }
> 373: };

`visit_range_in_order` looks okay, though it surely is a bit of a brain teaser. So the first half of the algorithm walks the outer-left flank of whatever the current head is, up to the point where we "stick a toe outside the range", then it repeats that part, basically, with the right node, which could be a way in, which is why we again have to walk the left flank.

I assume this is based on a paper, but trust you to have checked that. However, I would really like regression testing for this. Lots of range checks in gtest, please (can just be tacked onto what I did ask you for yesterday).

src/hotspot/share/nmt/virtualMemoryTracker.hpp line 33:

> 31: #include "nmt/allocationSite.hpp"
> 32: #include "nmt/nmtCommon.hpp"
> 33: #include "runtime/atomic.hpp"

Please add includes only where needed, directly. Let's not rely on indirect includes. Unless this is a remnant from some earlier version, then pls just remove it.

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

> 1: /*
> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

Be a dear and add us, please :-)

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

> 154:   if (to_be_deleted_inbetween_a_b.length() == 0 && LEQ_A_found) {
> 155:     // We must have smashed a hole in an existing region (or replaced it entirely).
> 156:     // LEQ_A - A - B - (some node >= B)

nit, clearer comment (a bit) since at first glance looks like substraction: `LEQ_A < A < B < (some node >= B)`. Alternatively, `LEQ_A [A, B) C`

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

> 196: 
> 197:   // Finally, we can register the new region [A, B)'s summary data.
> 198:   auto& rescom = diff.flag[NMTUtil::flag_to_index(metadata.flag)];

Can we please not use `auto` when unnecessary? IDEs have enough of a hard time understanding our code. `SingleDiff&`, please.

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

> 65: 
> 66:     Metadata(NativeCallStackStorage::StackIndex stack_idx, MEMFLAGS flag)
> 67:     : stack_idx(stack_idx), flag(flag) {}

I would assert here that with state=released, we only ever want to see mtNone.

src/hotspot/share/nmt/vmatree.hpp line 91:

> 89:     StateType type() const {
> 90:       return static_cast<StateType>(type_flag[0]);
> 91:     }

Proposal: provide `is_reserved()` and `is_committed()` and replace manual comparisons with the state enum with those. Easier on the eye.

src/hotspot/share/nmt/vmatree.hpp line 114:

> 112:     bool is_noop() {
> 113:       return (in.type() == StateType::Released && out.type() == StateType::Released) ||
> 114:              (in.type() == out.type() && Metadata::equals(in.metadata(), out.metadata()));

We require a released region to be mtNone, state Released. You ensure so below. So here, you should just need the second condition, no special handling for released regions needed.

src/hotspot/share/nmt/vmatree.hpp line 117:

> 115:     }
> 116:   };
> 117: 

Do `IntervalState` and `IntervalChange` need to be exposed in the header?

src/hotspot/share/nmt/vmatree.hpp line 151:

> 149: 
> 150:   SummaryDiff release_mapping(position from, position sz) {
> 151:     Metadata empty;

Just a nit, but instead of the Metadata::Metadata() ctor creating an empty object, could we possibly scrap the default ctor and have an explicit static constexpr Metadata empty with invalid stackindex and mtNone as ctor args? I find that nicer to read.

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

PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-2049875773
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596632543
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596633794
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596649353
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596665032
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596670430
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596647037
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596667575
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596645720
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596659540
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1596644523