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

[Johan Sjölen]

On Tue, 23 Apr 2024 13:03:45 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> 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/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()`.

Simplified with `pval.out.stack()`

> 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.

We should leave these inner functions be and any validation logic should be placed within the `MemTracker` class.

> 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?

The `reserved` amount is done through the regular MemTracker interface. To quote the PR:

>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.

> 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

Fixed.

> 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.

I prefer using a bit more space than having it 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.

I do prefer leaving this as it.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579228774
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579230473
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579231827
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579232385
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579233292
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1579232767