RFR: JDK-8291878: NMT: Malloc limits
Aleksey Shipilev
shade at openjdk.org
Tue Aug 23 17:06:13 UTC 2022
On Tue, 16 Aug 2022 11:14:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> (Second try, I withdrew the first patch https://github.com/openjdk/jdk/pull/9778 to do some other NMT improvements first)
>
> This PR introduces malloc limits, similar to what the ancient `-XX:MallocMaxTestWords` did intend. `MallocMaxTestWords` is broken, but the solution proposed in this PR works fine since it is based on NMT. I plan to remove `-XX:MallocMaxTestWords`, but in a separate RFE since it requires fiddling with compiler tests.
>
> ### Why this is useful:
>
> This allows us to generate a fatal error in the VM if the malloc amount - either globally or for a speicific NMT category - surpasses a given threshold.
>
> We recently analyzed [JDK-8291919](https://bugs.openjdk.org/browse/JDK-8291919), a jdk11u-specific regression that caused compiler arena OOMs. A switch to limit compiler-related mallocs would have been very nice to cause a fatal error in the compiler thread, together with a replay file. I first tried `MallocMaxTestWords`, but that's broken since it does not de-account memory allocations.
>
> In addition to customer scenarios like these, such a switch could be used to add sanity checks to compiler jtreg tests, adding malloc usage envelopes to tests. Maybe we could have caught [JDK-8291919](https://bugs.openjdk.org/browse/JDK-8291919) before shipment.
>
> ### How it works:
>
> Patch introduces a new diagnostic switch `-XX:MallocLimit`. That switch can be used in two ways:
>
> 1 impose a total global limit to the size hotspot is allowed to malloc:
>
> -XX:MallocLimit=<size>
>
> 2 impose a limit to a selected NMT category, or to multiple NMT categories:
>
> -XX:MallocLimit=<category>:<size>[,<category>:<size>...]
>
>
> If the switch is set, and the VM mallocs more in total (1) or for the given category (2), it will now stop with a fatal error. That way we can e.g. limit compiler arenas to a certain maximum in situations where the compiler runs amok, and get a compiler retry file. See here, with an artificial compiler bug introduced:
>
>
> thomas at starfish$ ./images/jdk/bin/java -XX:NativeMemoryTracking=summary -XX:+UnlockDiagnosticVMOptions -XX:MallocLimit=compiler:1g -jar /shared/projects/spring-petclinic/target/spring-petclinic-2.5.0-SNAP
> SHOT.jar
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # Internal Error (mallocTracker.cpp:146), pid=519822, tid=519836
> # guarantee(false) failed: MallocLimit: category "Compiler" reached limit (size: 1073765608, limit: 1073741824)
> #
> ...
> # An error report file with more information is saved as:
> # /shared/projects/openjdk/jdk-jdk/output-release/hs_err_pid519822.log
> #
> # Compiler replay data is saved as:
> # /shared/projects/openjdk/jdk-jdk/output-release/replay_pid519822.log
> #
>
>
> ### Costs:
>
> The feature requires that NMT checks the current number of allocated bytes via a given limit, on each malloc.
>
> - NMT disabled:
> - no costs
> - NMT enabled but malloc limits are not used
> - we pay a very small memory overhead (138 bytes)
> - we don't pay performance
> - NMT enabled and malloc limits are also enabled
> - if we have category-specific limits (e.g. `-XX:MallocLimit=compiler:1g`), the performance cost is so small it cannot be measured even in micro benchmarks. Which makes sense since we just compare two values that are likely to be cached by registers at the time of comparison.
> - Only if we have global limits (e.g. `-XX:MallocLimit=1g`) we pay a noticeable performance overhead. That is because on each malloc we need to add ~15 atomic counters to get the total malloc use (*). Noticeable means that with renaissance "philosophers" benchmark we see a performance drop of about 4%. This can be alleviated by future improvements of NMT that I have planned (only bottleneck for NMT improvement is the number of Reviewers willing to look at it).
I think it is basically fine, but I have many stylistic comments.
src/hotspot/share/runtime/arguments.cpp line 4412:
> 4410: Arguments::ArgsRange range = parse_memory_size(s, &limit, 1, SIZE_MAX);
> 4411: switch (range) {
> 4412: case ArgsRange::arg_in_range: *out = (size_t)limit; return true;
Style:
Suggestion:
case ArgsRange::arg_in_range:
*out = (size_t)limit;
return true;
src/hotspot/share/runtime/arguments.cpp line 4420:
> 4418: break;
> 4419: default:
> 4420: // fall through
This is not fall-through ;)
Suggestion:
src/hotspot/share/runtime/arguments.cpp line 4450:
> 4448: // it will return all found limits in the limits array.
> 4449: // 4) If option is malformed, it will exit the VM.
> 4450: // For (2) and (3), limits not affected by the switch will be set to 0.
This duplicates the comment at declaration, do you really need two copies?
src/hotspot/share/runtime/globals.hpp line 1369:
> 1367: \
> 1368: product(ccstr, MallocLimit, nullptr, DIAGNOSTIC, \
> 1369: "Limit malloc allocation size from hotspot (requires NMT). " \
"from VM" rather than "from hotspot"?
"This feature requires enabled NMT."?
src/hotspot/share/runtime/globals.hpp line 1373:
> 1371: "Usage:" \
> 1372: "- MallocLimit=<size> to set a total limit. " \
> 1373: "- MallocLimit=<NMT category>:<size>[,<NMT category>:<size>..] " \
Suggestion:
"- MallocLimit=<NMT category>:<size>[,<NMT category>:<size>...] " \
src/hotspot/share/services/mallocTracker.cpp line 105:
> 103:
> 104: if (_total_limit > 0) {
> 105: log_info(nmt)("MallocLimit: total limit: " SIZE_FORMAT, _total_limit);
We have a few useful utilities for printing byte sizes, see `byte_size_in_proper_unit` and friends. I assume limits are expected to be in integer megabytes, so this might print something very large.
src/hotspot/share/services/mallocTracker.cpp line 119:
> 117: // Assert in both debug and release, but allow error reporting to malloc beyond limits.
> 118: if (!VMError::is_error_reported()) {
> 119: guarantee(false,
Here and later, `guarantee(false, ...)` is just `fatal(...)`?
src/hotspot/share/services/nmtCommon.cpp line 92:
> 90: // Given a string, return associated flag. mtNone if name is invalid.
> 91: // String can be either the human readable name or the
> 92: // stringified enum (with or without leading "mt"). In all cases, case is ignored.
Comment is duplicated as declaration, leave one?
test/hotspot/jtreg/runtime/NMT/MallocLimitTest.java line 27:
> 25:
> 26: /*
> 27: * @test id=global_limit
I believe the convention is to use dashes, not underscores in test ids.
test/hotspot/jtreg/runtime/NMT/MallocLimitTest.java line 85:
> 83: public class MallocLimitTest {
> 84:
> 85: private static ProcessBuilder processBuilderWithSetting(String... extra_settings) {
Here and later, Java style: "extraSettings"
test/hotspot/jtreg/runtime/NMT/MallocLimitTest.java line 94:
> 92: String[] both = Stream.concat(Stream.concat(Arrays.stream(vmargs), Arrays.stream(extra_settings)), Arrays.stream(vmargs2))
> 93: .toArray(String[]::new);
> 94: ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(both);
Oof :)
Suggestion:
List<String> args = new ArrayList<>();
args.add("-XX:+UnlockDiagnosticVMOptions"); // MallocLimit is diagnostic
args.add("-Xmx64m");
args.add("-XX:-CreateCoredumpOnCrash");
args.add("-Xlog:nmt");
args.add("-XX:NativeMemoryTracking=summary");
args.addAll(Arrays.asList(extra_settings));
args.add("-version");
ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(args);
test/hotspot/jtreg/runtime/NMT/MallocLimitTest.java line 98:
> 96: }
> 97:
> 98: private static void test_global_limit() throws IOException {
Here and later, Java style: `testGlobalLimit`
test/hotspot/jtreg/runtime/NMT/MallocLimitTest.java line 102:
> 100: ProcessBuilder pb = processBuilderWithSetting("-XX:MallocLimit=" + small_memory_size);
> 101: OutputAnalyzer output = new OutputAnalyzer(pb.start());
> 102: output.reportDiagnosticSummary();
Do you really need `reportDiagnosticSummary()`? I thought it would be printed if `OutputAnalyzer` barfs on error.
-------------
PR: https://git.openjdk.org/jdk/pull/9891
More information about the hotspot-runtime-dev
mailing list