JDK-8320061: [nmt] Multiple issues with peak accounting #16675
Conversation
|
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
openjdk
bot
added
hotspot-runtime
|
/label remove core-libs |
|
@tstuefe |
Webrevs
|
|
Friendly ping. |
|
Ping @jdksjolen and @gerardziemski ? Would be nice to have this fixed in 22. |
|
Hi Thomas, I'll take a look. |
There was a problem hiding this comment.
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?
| // We omit printing altogether if: | ||
| // - the current reserved value falls below scale | ||
| // - none of the historic peaks (malloc, mmap committed, arena) raised above scale either |
There was a problem hiding this comment.
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.
| while (needleIdx < needles.length && haystackIdx < haystack.length && (started == false || found == true)) { | ||
| if (verbose) { | ||
| System.out.println("" + haystackIdx + ":" + haystack[haystackIdx]); | ||
| } | ||
| found = haystack[haystackIdx].contains(needles[needleIdx]); | ||
| if (found) { | ||
| if (verbose) { | ||
| System.out.println("Matches pattern " + needleIdx + " (\"" + haystack[haystackIdx] + "\")"); | ||
| } | ||
| started = true; | ||
| needleIdx++; | ||
| } | ||
| haystackIdx++; | ||
| } | ||
| if (needleIdx < needles.length) { | ||
| String err = "First unmatched: \"" + needles[needleIdx] + "\""; | ||
| if (!verbose) { // don't print twice | ||
| reportDiagnosticSummary(); | ||
| } | ||
| throw new RuntimeException(err); | ||
| } |
There was a problem hiding this comment.
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] + "\")");
}
}There was a problem hiding this comment.
Splitting this up into multiple loops would simplify the code significantly imho, no need for two boolean flags for a start.
If verbose=true, we print the haystack, with pattern matches marked. Your version omits printing for all lines until a needle is found, and only prints from the second needle on. Meaning, if the first pattern does not match, we don't see output. Arguably, that is the point where you need the output for test analysis.
Okay, I can give it a try.
Also, what does the !verbose check do exactly? Doesn't seem like it would be print twice without it, to me.
If an error happened, we print diagnostic summary (which mimics the behavior of all other "shouldBlaBla()" functions). So, we print twice if we run with verbose and run into an error.
There was a problem hiding this comment.
Okay, I rewrote it in the style you prefer, while keeping the logs I wanted intact; this does not look much clearer to me tbh. I found the first version more elegant, since it was a one-loop state machine and could be easily adapted to other uses (e.g. scanning for pattern that are not in directly consecutive lines, but interspersed)
No matter. I got it to work, lets keep it this way.
There was a problem hiding this comment.
If an error happened, we print diagnostic summary (which mimics the behavior of all other "shouldBlaBla()" functions). So, we print twice if we run with verbose and run into an error.
Thank you for explaining!
I found the first version more elegant, since it was a one-loop state machine and could be easily adapted to other uses (e.g. scanning for pattern that are not in directly consecutive lines, but interspersed)
That's just differing taste then, thank you for taking the effort to try out my idea :-).
…-historic-peak-raises-above-scale-threshold
Hi Johan, thanks for taking a look!
The tests needed to be adapted, since they all expected entries to vanish when their current value is low. But that's exactly what this patch tries to fix: to preserve all the historical peaks. Because of that, a bunch of tests broke. I only touched tests that had been broken by the NMT change. I did not change other tests. I did, however, add more tests for detailled "mmap" and "arena" numbers) |
|
@jdksjolen are we good? (sorry, 22 deadline nears) |
|
@tstuefe This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Thanks @jdksjolen ! Anyone else willing to give a second review? @gerardziemski maybe? |
There was a problem hiding this comment.
seems a couple of tests need the copyright year update like test/hotspot/jtreg/runtime/NMT/MallocRoundingReportTest.java .
Otherwise looks okay.
Many thanks, Matthias! Copyrights fixed. I'll wait for a last GHA run, then push. |
…-historic-peak-raises-above-scale-threshold
|
/integrate |
|
Going to push as commit dc256fb.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
There are multiple issues with peak printing.
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=gonly 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.
We decided to make peak printing a release-build feature with JDK-8317772, but peak printing for virtual memory is still debug-only
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)Finally, we really should have better regression tests for peak accounting.
This patch fixes points (1)..(3).
The majority of changes affect test cases, though, because many tests expect NMT to print nothing for allocations that are close to or at zero. But since we now print historical peaks even if current values are low (which is the point of this patch) these tests break.
I found that we have a lot of duplicated coding in the tests and reduced that with some common utility classes.
Tests:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16675/head:pull/16675$ git checkout pull/16675Update a local copy of the PR:
$ git checkout pull/16675$ git pull https://git.openjdk.org/jdk.git pull/16675/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 16675View PR using the GUI difftool:
$ git pr show -t 16675Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16675.diff
Webrev
Link to Webrev Comment