RFR: JDK-8320061: [nmt] Multiple issues with peak accounting
Johan Sjölen
jsjolen at openjdk.org
Fri Nov 24 15:31:14 UTC 2023
On Wed, 15 Nov 2023 10:06:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> There are multiple issues with peak printing.
>
> 1) Peak values are not considered when deciding whether allocations fall above the scale threshold:
>
> NMT omits printing information if the values, expressed with the current NMT scale, don't rise above 0. For example, `jcmd myprog VM.native_memory scale=g` only shows allocations above 1GB. However, we should also show information that had *historically* large values, even if their current value is small or even 0.
>
> A typical example is compiler memory usage, which can reach large peaks during VM initialization and may fall to insignificant numbers afterward. We still want to see those peaks.
>
> 2) We decided to make peak printing a release-build feature with [JDK-8317772](https://bugs.openjdk.org/browse/JDK-8317772), but peak printing for virtual memory is still debug-only
>
> 3) There is a bug in that VirtualMemory::peak that causes the peak value to be wrong; it gets not updated from the actual peak but from the commit increase, so the value shown is too low if we committed in multiple steps. That can be simply observed by "largest_committed" being smaller than "committed", e.g.: `(mmap: reserved=1048576KB, committed=12928KB, largest_committed=64KB)`
>
> 4) Finally, we really should have better regression tests for peak accounting.
>
> ----
>
> This patch fixes points (1)..(3). It also adds regression tests for peak printing to all affected existing NMT tests. I found that we have a lot of duplicated coding in the tests and reduced that with some common utility classes.
>
> Tests:
> - locally tested runtime/NMT and the full gtest suite on x64 release, fastdebug and x86 fastdebug.
> - GHA
The HotSpot code looks good generally. The test utilities are nice to see.
I've looked through the test changes, but it seems that some change what the test does while some simply refactor the tests to use the new utilities. It would have been nice to have those in separate commtis to make reviewing easier. What is the idea behind changing the tests?
src/hotspot/share/nmt/memReporter.cpp line 215:
> 213: // We omit printing altogether if:
> 214: // - the current reserved value falls below scale
> 215: // - none of the historic peaks (malloc, mmap committed, arena) raised above scale either
We omit printing altogether if all of the following values fall below the scale:
- the current reserved value falls below scale
- the historic peaks (malloc, mmap, committed, arena)
Perhaps this reads a bit better, rather than inversing the condition in the first bullet when writing out the second one.
src/hotspot/share/nmt/memReporter.cpp line 220:
> 218: const size_t pk_arena = malloc_memory->arena_peak_size();
> 219:
> 220: if (amount_in_current_scale(MAX4(reserved_amount, pk_vm, pk_malloc, pk_arena)) > 0) {
This branch body is very long and there is no `else`, can we invert the condition and do an early return instead?
src/hotspot/share/nmt/memReporter.cpp line 336:
> 334: // Omit printing if neither the current nor the historic peak malloc amount raises above
> 335: // reporting scale threshold.
> 336: if (amount_in_current_scale(MAX2(malloc_site->size(), malloc_site->peak_size())) == 0) {
This comment reads as if there are two peaks, one current and one historic. Just go for the simpler "Omit printing if the current value and the historic peak value both fall below the reporting scale threshold". Same with the other one.
src/hotspot/share/nmt/memReporter.cpp line 367:
> 365: }
> 366: // Omit printing if neither the current nor the historic peak malloc amount raises above
> 367: // reporting scale threshold.
The comment is also wrong, this is not about malloc amount!
src/hotspot/share/nmt/memReporter.cpp line 410:
> 408: // usage *by callsite*.
> 409:
> 410: // Don't report if size is too small.
Yes, this makes sense.
test/hotspot/jtreg/runtime/NMT/HugeArenaTracking.java line 82:
> 80: wb.NMTFreeArena(arena1);
> 81:
> 82: // Repeat report at GB level. Peak should be 0 now. Current usage is 1KB, since arena2 is left, but that
"Peak should be 0 now" <- that's gotta be incorrect.
test/hotspot/jtreg/runtime/NMT/MallocTestType.java line 52:
> 50: wb.NMTFree(memAlloc3); // current +256K #1 peak +384K #2
> 51: long memAlloc1 = wb.NMTMalloc(512 * 1024); // current +768K #2 peak +768K #2
> 52: wb.NMTFree(memAlloc2); // current +512K #1 peak +768K #2
Comment indent style
test/hotspot/jtreg/runtime/NMT/MallocTestType.java line 59:
> 57:
> 58: // Free the memory allocated by NMTAllocTest
> 59: wb.NMTFree(memAlloc1); // current 0K #0 peak +768K #2
I guess this is to match the comment indent from before? I don't think it should be indented to match previous comments as there is code in the way.
test/lib/jdk/test/lib/process/OutputAnalyzer.java line 822:
> 820: }
> 821: throw new RuntimeException(err);
> 822: }
Splitting this up into multiple loops would simplify the code significantly imho, no need for two boolean flags for a start. Also, what does the `!verbose` check do exactly? Doesn't seem like it would be print twice without it, to me.
if (needles.length == 0) return;
int haystackStart = 0;
for (int i = 0; i < haystack.length; i++) {
if (haystack[i].contains(needles[0])) {
hayStackStart = i+1;
return;
}
}
for (int i = 1; i < needles.length; i++) {
if (verbose && haystackStart + i < haystack.length) {
System.out.println("" + haystackStart + i + ":" + haystack[haystackStart + i]);
}
if (haystackStart + i >= haystack.length
|| !haystack[haystackStart + i].contains(needles[i])) {
String err = "First unmatched: "" + needles[needleIdx] + """;
if (!verbose) { // don't print twice
reportDiagnosticSummary();
}
throw new RuntimeException(err);
}
if (verbose) {
System.out.println("Matches pattern " + needleIdx + " ("" + haystack[haystackStart + i] + "")");
}
}
-------------
PR Review: https://git.openjdk.org/jdk/pull/16675#pullrequestreview-1748059774
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404434037
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404422479
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404436216
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404437690
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404425988
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404440477
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404441509
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404442564
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404463804
More information about the hotspot-runtime-dev
mailing list