RFR: 8312132: Add tracking of multiple address spaces in NMT [v46]
Afshin Zafari
azafari at openjdk.org
Tue Apr 23 15:43:47 UTC 2024
On Mon, 22 Apr 2024 16:00:51 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 two additional commits since the last revision:
>
> - Remove faulty condition after removing merging
> - Add failing test case
src/hotspot/share/gc/z/zInitialize.cpp line 1:
> 1: /*
Copyright is to be updated.
src/hotspot/share/gc/z/zNMT.cpp line 45:
> 43:
> 44: void ZNMT::commit(zoffset offset, size_t size) {
> 45: MemTracker::allocate_memory_in(ZNMT::_device, untype(offset), size, mtJavaHeap, CALLER_PC);
`NativeCallStack` param should be before the `MEMFLAGS` param, same as other functions.
src/hotspot/share/nmt/memBaseline.cpp line 1:
> 1: /*
Copyright to be updated.
src/hotspot/share/nmt/memReporter.cpp line 1:
> 1: /*
Copyright.
src/hotspot/share/nmt/memReporter.hpp line 1:
> 1: /*
Copyright.
src/hotspot/share/nmt/memTracker.cpp line 1:
> 1: /*
Copyright.
src/hotspot/share/nmt/memTracker.hpp line 1:
> 1: /*
Copyright.
src/hotspot/share/nmt/memTracker.hpp line 177:
> 175: }
> 176:
> 177: static inline void remove_device(MemoryFileTracker::MemoryFile* device) {
check device != nullptr.
src/hotspot/share/nmt/memTracker.hpp line 184:
> 182: }
> 183:
> 184: static inline void allocate_memory_in(MemoryFileTracker::MemoryFile* device, size_t offset, size_t size,
invalid args: `nullptr` and `size == 0`.
src/hotspot/share/nmt/memTracker.hpp line 192:
> 190: }
> 191:
> 192: static inline void free_memory_in(MemoryFileTracker::MemoryFile* device,
same here.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 43:
> 41: }
> 42:
> 43: void MemoryFileTracker::allocate_memory(MemoryFile* device, size_t offset,
check/assert `device == nullptr`.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 45:
> 43: void MemoryFileTracker::allocate_memory(MemoryFile* device, size_t offset,
> 44: size_t size, MEMFLAGS flag,
> 45: const NativeCallStack& stack) {
indentation does not match with the line above.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 47:
> 45: const NativeCallStack& stack) {
> 46: NativeCallStackStorage::StackIndex sidx = _stack_storage.push(stack);
> 47: DeviceSpace::Metadata metadata(sidx, flag);
Can `Metadata` ctor gets a `NaticeCallStack` instead of an index? StackIndex is not used for the rest of the code.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 48:
> 46: NativeCallStackStorage::StackIndex sidx = _stack_storage.push(stack);
> 47: DeviceSpace::Metadata metadata(sidx, flag);
> 48: DeviceSpace::SummaryDiff diff = device->_tree.reserve_mapping(offset, size, metadata);
What if `size == 0`?
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 52:
> 50: const VMATree::SingleDiff& rescom = diff.flag[i];
> 51: VirtualMemory* summary = device->_summary.by_type(NMTUtil::index_to_flag(i));
> 52: summary->reserve_memory(rescom.reserve);
`diff.flag[i]` can be used instead of `rescom` and the corresponding line can be removed.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 56:
> 54: }
> 55:
> 56: void MemoryFileTracker::free_memory(MemoryFile* device, size_t offset, size_t size) {
same comments here.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 65:
> 63: }
> 64:
> 65: void MemoryFileTracker::print_report_on(const MemoryFile* device, outputStream* stream, size_t scale) {
check for invalid arguments: `nullptr` and `0`.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 85:
> 83: NMTUtil::scale_name(scale),
> 84: NMTUtil::flag_to_name(pval.out.metadata().flag));
> 85: pval.out.metadata().stack_idx.stack().print_on(stream, 4);
Why hard coded `4`? Is it the depth of stack?
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 85:
> 83: NMTUtil::scale_name(scale),
> 84: NMTUtil::flag_to_name(pval.out.metadata().flag));
> 85: pval.out.metadata().stack_idx.stack().print_on(stream, 4);
Also, if `IntervalChange` has some wrappers we can write:
`pval.out_stack().print_on()` or `pval.out_type()`.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 121:
> 119: }
> 120:
> 121: void MemoryFileTracker::Instance::allocate_memory(MemoryFile* device, size_t offset,
check invalid args.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 123:
> 121: void MemoryFileTracker::Instance::allocate_memory(MemoryFile* device, size_t offset,
> 122: size_t size, MEMFLAGS flag,
> 123: const NativeCallStack& stack) {
indentation ...
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 124:
> 122: size_t size, MEMFLAGS flag,
> 123: const NativeCallStack& stack) {
> 124: _tracker->allocate_memory(device, offset, size, flag, stack);
`_tracker` can be `nullptr` if `initialize` is not called or if it failed to allocate. Maybe `!enabled()` is to be used here to check it.
This also applies for any further use of `_tracker` in the subsequent functions.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 127:
> 125: }
> 126:
> 127: void MemoryFileTracker::Instance::free_memory(MemoryFile* device, size_t offset,
check of invalid args.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 128:
> 126:
> 127: void MemoryFileTracker::Instance::free_memory(MemoryFile* device, size_t offset,
> 128: size_t size) {
indentation.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 138:
> 136:
> 137: void MemoryFileTracker::Instance::print_report_on(const MemoryFile* device,
> 138: outputStream* stream, size_t scale) {
indentation.
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 152:
> 150: auto snap = snapshot->by_type(NMTUtil::index_to_flag(i));
> 151: auto current = device->_summary.by_type(NMTUtil::index_to_flag(i));
> 152: // PDT stores the memory as reserved but it's accounted as committed.
What does PDT stand for?
src/hotspot/share/nmt/nmtMemoryFileTracker.cpp line 153:
> 151: auto current = device->_summary.by_type(NMTUtil::index_to_flag(i));
> 152: // PDT stores the memory as reserved but it's accounted as committed.
> 153: snap->commit_memory(current->reserved());
`VirtualMemorySnapshot` contains both `reserved` and `committed` amounts. If `current->reserved()` is used for `committed` amount, what is the `reserved` amount then?
src/hotspot/share/nmt/nmtMemoryFileTracker.hpp line 49:
> 47:
> 48: // Each device has its own memory space.
> 49: using DeviceSpace = VMATree;
`DeviceSpace` is used only 3 times in 2 methods in cpp file. `VMATree` is also used all around the code.
`VMATree` is preferable then.
src/hotspot/share/nmt/nmtMemoryFileTracker.hpp line 62:
> 60: MemoryFile(const char* descriptive_name)
> 61: : _descriptive_name(descriptive_name) {
> 62: }
can fit into 1 line.
src/hotspot/share/nmt/nmtTreap.hpp line 26:
> 24:
> 25: #ifndef SHARE_NMT_TREAP_HPP
> 26: #define SHARE_NMT_TREAP_HPP
SHARE_NMT_NMTTREAP_HPP
src/hotspot/share/nmt/nmtTreap.hpp line 285:
> 283: using TreapCHeap = Treap<K, V, COMPARATOR, TreapCHeapAllocator>;
> 284:
> 285: #endif //SHARE_NMT_TREAP_HPP
SHARE_NMT_NMTTREAP_HPP
src/hotspot/share/nmt/virtualMemoryTracker.hpp line 1:
> 1: /*
Copyright.
src/hotspot/share/nmt/vmatree.cpp line 36:
> 34: MEMFLAGS flag_out() const {
> 35: return state.out.metadata().flag;
> 36: }
can fit in 1 line.
src/hotspot/share/nmt/vmatree.cpp line 42:
> 40: // Motivating example: reserve(0,100, mtNMT); reserve(50,75, mtTest);
> 41: // This will require the 2nd call to know which region the second reserve 'smashes' a hole into for proper summary accounting.
> 42: // LEQ_A is figured out a bit later on, as we need to find it for other purposes anyway.
Let's have a right margin for this part, at column 85 for example.
src/hotspot/share/nmt/vmatree.cpp line 188:
> 186: // LEQ_A - A - B - GEQ_B
> 187: auto& rescom = diff.flag[NMTUtil::flag_to_index(LEQ_A.flag_out())];
> 188: if (LEQ_A.state.out.type() == StateType::Reserved) {
Would be nice to have wrappers that allow us write these as:
`LEQ_A.is_out_reserved()`
or
`LEQ_A.is_out_committed()`
src/hotspot/share/nmt/vmatree.cpp line 201:
> 199: });
> 200:
> 201: AddressState prev = {A, stA}; // stA is just filler
`AddressState prev{A, stA};` would be like other instances of ctor. I.e., remove the `=` sign.
src/hotspot/share/nmt/vmatree.cpp line 242:
> 240: }
> 241: return diff;
> 242: }
Would be nice if we can break this function into some smaller sub-functions. It is 200+ line now and little hard to track the logic. Thanks!
src/hotspot/share/nmt/vmatree.hpp line 44:
> 42: class AddressComparator {
> 43: public:
> 44: static int cmp(size_t a, size_t b) {
Why `size_t` and not `address`?
src/hotspot/share/nmt/vmatree.hpp line 56:
> 54:
> 55: // Each point has some stack and a flag associated with it.
> 56: struct Metadata {
`State` and `Metadata` are attributes of a Node and not to be in VMATree.
src/hotspot/share/nmt/vmatree.hpp line 63:
> 61: : stack_idx(),
> 62: flag(mtNone) {
> 63: }
can fit in 1 line.
src/hotspot/share/nmt/vmatree.hpp line 70:
> 68: static bool equals(const Metadata& a, const Metadata& b) {
> 69: return NativeCallStackStorage::StackIndex::equals(a.stack_idx, b.stack_idx) &&
> 70: a.flag == b.flag;
`a.flag == b.flag` can be left-hand of `&&` to be more efficient.
src/hotspot/share/nmt/vmatree.hpp line 113:
> 111: };
> 112:
> 113: using VTreap = TreapNode<size_t, IntervalChange, AddressComparator>;
Why `VTreap` and not `TreapNode`? What does the `V` alone say?
src/hotspot/share/nmt/vmatree.hpp line 118:
> 116: VMATree()
> 117: : tree() {
> 118: }
fit into 1 line.
src/hotspot/share/nmt/vmatree.hpp line 135:
> 133: SummaryDiff register_mapping(size_t A, size_t B, StateType state, Metadata& metadata);
> 134:
> 135: SummaryDiff reserve_mapping(size_t from, size_t sz, Metadata& metadata) {
If we use `reserve_mapping` for `uncommit_memory`, we need to set a `StackIndex` and a `MEMFLAGS` to pass as a `Metadata`. If we use `mtNone` for example, all the uncommitted amount would be accounted for `mtNone`.
Would you please provide a `uncommit_mapping(address, size)` to handle these issues properly?
src/hotspot/share/nmt/vmatree.hpp line 139:
> 137: }
> 138:
> 139: SummaryDiff commit_mapping(size_t from, size_t sz, Metadata& metadata) {
`size_t` or `address` for `from`?
src/hotspot/share/nmt/vmatree.hpp line 145:
> 143: SummaryDiff release_mapping(size_t from, size_t sz) {
> 144: Metadata empty;
> 145: return register_mapping(from, from + sz, StateType::Released, empty);
`return register_mapping(from, from + sz, StateType::Released, Metadata{});`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576183035
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576230158
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576236166
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576237931
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576240387
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576242164
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576244061
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576246868
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576247885
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576248209
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576191957
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576254696
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576196658
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576201734
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576203893
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576205419
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576207394
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576213967
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576219519
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576256738
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576255953
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576278534
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576265787
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576257264
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576281076
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576285582
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576287500
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576297271
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576300493
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576310421
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576324616
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576325346
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576328824
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576328036
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576352240
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576349129
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576360411
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576362112
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576399517
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576363348
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576371244
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576408517
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576404851
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576387513
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576403278
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1576388843
More information about the hotspot-dev
mailing list