RFR: 8317453: NMT: Performance benchmarks are needed to measure speed and memory [v16]

[Johan Sjölen]

On Thu, 8 May 2025 14:47:46 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> Please review this addition of an internal benchmark, mostly of interest to those working with NMT.
>> 
>> This benchmark allows us to record a pattern of memory allocation operations (i.e. `malloc`, `realloc` and `free`) as well as the virtual memory allocations (i.e. `VirtualMemoryTracker::add_reserved_region`, etc.) and record those into files.
>> 
>> Later we can use that recording to _play back_ the pattern with different code or settings to compare the performance (i.e. memory usage as well as time).
>> 
>> The goal of this benchmark is for anyone working on NMT to be able to measure and prove whether their improvement helps or regresses the performance.
>> 
>> ### To use it:
>> 
>> To record pattern of allocations of memory calls:
>> 
>> `NMTRecordMemoryAllocations=0x7FFFFFFF ./build/macosx-aarch64-server-release/xcode/build/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:NativeMemoryTracking=summary -jar build/macosx-aarch64-server-release/images/jdk/demo/jfc/J2Ddemo/J2Ddemo.jar`
>> 
>> OR to record pattern of allocations of virtual memory calls:
>> 
>> `NMTRecordVirtualMemoryAllocations=0x7FFFFFFF ./build/macosx-aarch64-server-release/xcode/build/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:NativeMemoryTracking=summary -jar build/macosx-aarch64-server-release/images/jdk/demo/jfc/J2Ddemo/J2Ddemo.jar`
>> 
>> This will result in the file:
>> -  hs_nmt_pid22770_allocs_record.log (is the chronological record of the the desired operations)
>> OR
>> - hs_nmt_pid22770_virtual_allocs_record.log (is the chronological record of the desired operations)
>> 
>> And 2 additional files:
>> - hs_nmt_pid22770_info_record.log (is the record of default NMT memory overhead and the NMT state)
>> - hs_nmt_pid22770_threads_record.log (is the record of thread names that can be retrieved later when processing)
>> 
>> 
>> then to actually run the benchmark:
>> 
>> NMTBenchmarkRecordedPID=22770 ./build/macosx-aarch64-server-release/xcode/build/jdk/bin/java -XX:+UnlockDiagnosticVMOptions -XX:NativeMemoryTracking=summary
>> 
>> ### Usage:
>> 
>> See the issue for more details and the design document.
>
> Gerard Ziemski has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Johan's feedback

src/hotspot/share/nmt/memLogRecorder.cpp line 105:

> 103:     }
> 104:     NMT_MemoryLogRecorder::initialize(count);
> 105:   }

Why do we do `strtol` if `atol` fails? What if `strtol` fails?

src/hotspot/share/nmt/memLogRecorder.cpp line 193:

> 191: #define IS_FREE(e)           ((e->requested == 0) && (e->old == nullptr))
> 192: #define IS_REALLOC(e)        ((e->requested  > 0) && (e->old != nullptr))
> 193: #define IS_MALLOC(e)         ((e->requested  > 0) && (e->old == nullptr))

Make into functions

src/hotspot/share/nmt/memLogRecorder.cpp line 200:

> 198: #define BENCHMARK_LOG_FILE "hs_nmt_pid%p_benchmark.log"
> 199: #define VALLOCS_LOG_FILE "hs_nmt_pid%p_virtual_allocs_record.log"
> 200: 

Please replace all `#define`s with constants instead, do not use `ALL_CAPS` formatting for the replacements.

src/hotspot/share/nmt/memLogRecorder.cpp line 227:

> 225:   if (!IS_VALID_FD(fd)) {
> 226:     tty->print("write_and_check(%d) ERROR\n", fd);
> 227:     //assert(false, "fd: %d", fd);

Please remove any commented out asserts that you do not intend to keep.

src/hotspot/share/nmt/memLogRecorder.cpp line 364:

> 362: 
> 363: void NMT_MemoryLogRecorder::replay(const int pid) {
> 364:   // tty->print("NMT_MemoryLogRecorder::replay(%d)\n", pid);

Please make sure that you've removed all commented out prints if you don't intend to keep them.

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

> 92:     NMT_MemoryLogRecorder::record_alloc(mem_tag, size, mem_base, &stack, old_base);
> 93:     return ptr;
> 94:   }

I'm not a fan of making `record_malloc` aware of the fact that it may be used within a `realloc` context with the addition of the `old_base`. I'd like to see this parameter removed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097430324
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097436905
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097436503
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097437914
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097432742
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2097423738