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

[Johan Sjölen]

On Thu, 1 May 2025 14:12:30 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:
> 
>   use permit_forbidden_function for realloc

There's commented out code, fix that by getting rid of it or converting logs to UL.
 
Sometimes you use `fprintf(stderr`, sometimes you use `tty->print`, what's the difference and why not use UL?

src/hotspot/share/nmt/mallocTracker.cpp line 170:

> 168: // Record a malloc memory allocation
> 169: void* MallocTracker::record_malloc(void* malloc_base, size_t size, MemTag mem_tag,
> 170:   const NativeCallStack& stack, void* old_base)

Unused

src/hotspot/share/nmt/mallocTracker.hpp line 284:

> 282:   // Record  malloc on specified memory block
> 283:   static void* record_malloc(void* malloc_base, size_t size, MemTag mem_tag,
> 284:     const NativeCallStack& stack, void* old_base = nullptr);

Unused

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

> 153:   // TODO: NMT_LogRecorder::thread_name
> 154: #endif
> 155: }

`Thread::current()->name()`

src/hotspot/share/nmt/memLogRecorder.hpp line 136:

> 134:     address stack[NMT_TrackingStackDepth];
> 135:     long int mem_tag;
> 136:     long int mem_tag_split;

Use MemTag? Why `long int`?

src/hotspot/share/nmt/memLogRecorder.hpp line 139:

> 137:     size_t size;
> 138:     size_t size_split;
> 139:     int type;

Why isn't this a `Type`?

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

> 149:     SPLIT_RESERVED,
> 150:     TAG
> 151:   };

Better name than `Type`, like `MemoryOperation`? No need for `ALL_CAPS` names if you don't want to, you can use `ThisTypeOfName` instead. That's a style choice you get to make, though.

src/hotspot/share/nmt/memLogRecorder.hpp line 174:

> 172: #else // defined(LINUX) || defined(__APPLE__)
> 173: 
> 174: class NMT_LogRecorder : public StackObj {

What's the idea behind having two different subclasses for the log recorder? Like, why is it important that two different objects record the two sequences of events?

src/hotspot/share/nmt/memLogRecorder.hpp line 185:

> 183: 
> 184: class NMT_MemoryLogRecorder : public NMT_LogRecorder {
> 185: public:

TODOs?

src/hotspot/share/runtime/os.cpp line 739:

> 737:     // After a successful realloc(3), we account the resized block with its new size
> 738:     // to NMT.
> 739:     void* const new_inner_ptr = MemTracker::record_malloc(new_outer_ptr, size, mem_tag, stack, memblock);

Unused extra argument

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

PR Review: https://git.openjdk.org/jdk/pull/23786#pullrequestreview-2814746682
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073411322
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073411520
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073423794
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073415653
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073416162
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073417724
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073414948
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073412512
PR Review Comment: https://git.openjdk.org/jdk/pull/23786#discussion_r2073418728