RFR: 8312132: Add tracking of multiple address spaces in NMT [v105]
Thomas Stuefe
stuefe at openjdk.org
Fri May 24 08:06:17 UTC 2024
On Wed, 22 May 2024 14:12:44 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:
>
> Lower number of pages
Looked at native callstack storage, memfiletracker, some other stuff.
Will continue next week.
src/hotspot/share/nmt/memReporter.cpp line 879:
> 877: }
> 878:
> 879: void MemDetailReporter::report_physical_devices() {
Can we rename this? This is really not about physical devices. "report_address_spaces" ? Also not better. Sigh.
"report_anonymous_kernel_memory_allocated_on_behalf_of_ZGC_with_memfd_that_we_dont_see_in_rss_ever" ?
But seriously, maybe "report_memory_file_allocations"? That would work.
src/hotspot/share/nmt/memoryFileTracker.cpp line 50:
> 48: for (int i = 0; i < mt_number_of_types; i++) {
> 49: VirtualMemory* summary = device->_summary.by_type(NMTUtil::index_to_flag(i));
> 50: summary->reserve_memory(diff.flag[i].reserve);
Why do we only track reserved memory here?
src/hotspot/share/nmt/memoryFileTracker.cpp line 51:
> 49: VirtualMemory* summary = device->_summary.by_type(NMTUtil::index_to_flag(i));
> 50: summary->reserve_memory(diff.flag[i].reserve);
> 51: }
This seems to be a recurring pattern.
- Please make VMATree::SummaryDiff a first-class NMT class. Maybe rename it, too, since it does not necessarily have anything to do with Summary reports. Maybe something like like "MemoryDeltas", being an array of "MemoryDelta". Up to you.
- Then give us something like "VirtualMemorySnapshot.apply_delta(MemoryDeltas)".
That saves code, and when reading its callsite, your intent is much clearer.
src/hotspot/share/nmt/memoryFileTracker.cpp line 66:
> 64: stream->cr();
> 65: VMATree::TreapNode* prev = nullptr;
> 66: device->_tree.visit_in_order([&](VMATree::TreapNode* current) {
Does MemoryFileTracker really need to be friend to access the tree? It only needs read-only access to the tree, nothing else. Why not expose a ro access to the tree?
I balk at the many friend relationships. To my eye, they undermine the encapsulation. I can see the point for test classes, but here?
src/hotspot/share/nmt/memoryFileTracker.cpp line 72:
> 70: return;
> 71: }
> 72: assert(prev->val().out.type() == current->val().in.type(), "must be");
Slight modification, since I expect we will stare at the output of this function to analyse broken trees. Please keep record of "brokenness" and assert at the end only. And print out the current number of the mapping, too. Then, on assert, print out "tree broken first at record XXX".
src/hotspot/share/nmt/memoryFileTracker.cpp line 157:
> 155: void MemoryFileTracker::summary_snapshot(VirtualMemorySnapshot* snapshot) const {
> 156: for (int d = 0; d < _devices.length(); d++) {
> 157: auto& device = _devices.at(d);
Lets make this a type, and const: `const MemoryFile*`
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 61:
> 59: : _stack_index(-1) {
> 60: }
> 61: };
If you want to retain StackIndex as class (there is no pressing need, could be just as well a simple typedef to a 32bit int), let's make it work for its money. E.g., in StackIndex(i), assert i >= 0. Maybe give it an is_invalid() method, and replace manual comparisons with -1 with index.is_invalid().
My obsessive compulsive mind nags at wasting half of the value range for "invalid". Granted, that only matters should we want to shrink the index to 16bit. Still, you could make the index uint32_t and declare UINT_MAX to be the invalid value. I know part of the reason is that GrowableArray is hardcoded to signed int as index, but no need for that to define the index type here.
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 80:
> 78: }
> 79: link = link->next;
> 80: }
Good. We do an youngest-first search if I am seeing right. Was that deliberate? The chance of the most recent callstacks reoccurring is a lot higher than seeing older stacks.
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 88:
> 86:
> 87: // For storage of the Links
> 88: Arena _arena;
I like that we store the links in arena, it saves space and mallocs. Only thing to remember is that now we will see "Arena" memory in the NMT category when printing the report.
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 92:
> 90: // 4099 gives a 50% probability of collisions at 76 stacks (as per birthday problem).
> 91: static const constexpr int default_nr_buckets = 4099;
> 92: int _nr_buckets;
isn't this normally called table_size or somesuch? _nr_buckets sounds like number of items, which this is not.
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 95:
> 93: Link** _buckets;
> 94: GrowableArrayCHeap<NativeCallStack, mtNMT> _stacks;
> 95: bool _is_detailed_mode;
_is_detailed: I somehow don't think this rather low level class should care about and copy the MemTracker state. I like "one truth only", which is MemTracker::enabled. I'd rather see this handled at the call site.
If we only need it to prevent allocation of the bucket table at construction time, I'd allocate that one with malloc.
src/hotspot/share/nmt/nmtNativeCallStackStorage.hpp line 119:
> 117: }
> 118: }
> 119: };
Possibly for follow up RFE: I would like to see number of stacks in the NMT statistic (there is this statistic subcommand to the jcmd VM.native_memory). I also would like to see those statistics in the hs-err file.
For example, if we ever decide to track larger stacks (which would make a lot of sense, 4 frames is really not much), we will see a logarithmic (?) increase in number of stacks. I would like to know those numbers. Note that I sometimes do that during investigations, and I have a RFE open somewhere to make the number of frames in stacks tunable with a VM options.
src/hotspot/share/nmt/vmatree.hpp line 146:
> 144: struct SingleDiff {
> 145: int64_t reserve;
> 146: int64_t commit;
The typical type would be `ssize_t`, not int64.
Apart from clarity, I am not sure how int64 would work on 32-bit.
test/hotspot/gtest/nmt/test_nmt_memoryfiletracker.cpp line 53:
> 51: TEST_VM_F(MemoryFileTrackerTest, Basics) {
> 52: this->basics();
> 53: }
Curious, just a question. You like using fixture classes even if not necessary. Why not write the test directly into a TEST_VM ?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-2075935829
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612991632
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613035196
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613015946
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613023874
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613027118
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613029549
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612953300
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612946445
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612964659
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612971831
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612960587
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612986848
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1613018290
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1612912160
More information about the hotspot-dev
mailing list