On Thu, 13 Jul 2023 18:56:56 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> This is a continuation of https://github.com/openjdk/jdk/pull/10085. I >> closed https://github.com/openjdk/jdk/pull/10085 because it had accumulated >> too much comment history and got confusing. For a history of this issue, see >> previous discussions [1] and the comment section of 10085. >> >> --------------- >> >> This RFE adds the option to trim the Glibc heap periodically. This can >> recover a significant memory footprint if the VM process suffers from >> high-but-rare malloc spikes. It does not matter who causes the spikes: the >> JDK or customer code running in the JVM process. >> >> ### Background: >> >> The Glibc is reluctant to return memory to the OS. Temporary malloc spikes >> often carry over as permanent RSS increase. Note that C-heap retention is >> difficult to observe. Since it is freed memory, it won't appear in NMT; it >> is just a part of RSS. >> >> This is, effectively, caching - a performance tradeoff by the glibc. It >> makes a lot of sense with applications that cause high traffic on the >> C-heap. The JVM, however, clusters allocations and often rolls its own >> memory management based on virtual memory for many of its use cases. >> >> To manually trim the C-heap, Glibc exposes `malloc_trim(3)`. With JDK 18 >> [2], we added a new jcmd command to *manually* trim the C-heap on Linux >> (`jcmd System.trim_native_heap`). We then observed customers running this >> command periodically to slim down process sizes of container-bound jvms. >> That is cumbersome, and the JVM can do this a lot better - among other >> things because it knows best when *not* to trim. >> >> #### GLIBC internals >> >> The following information I took from the glibc source code and >> experimenting. >> >> ##### Why do we need to trim manually? Does the Glibc not trim on free? >> >> Upon `free()`, glibc may return memory to the OS if: >> - the returned block was mmap'ed >> - the returned block was not added to tcache or to fastbins >> - the returned block, possibly merged with its two immediate neighbors, had >> they been free, is larger than FASTBIN_CONSOLIDATION_THRESHOLD (64K) - in >> that case: >> a) for the main arena, glibc attempts to lower the brk() >> b) for mmap-ed heaps, glibc attempts to completely unmap or shrink the >> heap. >> In both cases, (a) and (b), only the top portion of the heap is reclaimed. >> "Holes" in the middle of other in-use chunks are not reclaimed. >> >> So: glibc *may* automatically reclaim memory. In normal configurations, with >> typical C-heap allocation granularity, it is unlikely. >> >> To increase the ... > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > Bikeshed Trim log lines How have you determined all of the places that need `NativeHeapTrimmer::SuspendMark`? Couple of minor comments below but otherwise this seems good. I'll take it for a spin through our CI. Thanks src/hotspot/share/runtime/globals.hpp line 1992: > 1990: "the platform supports that. Lower values will reclaim memory > " \ > 1991: "more eagerly at the cost of higher overhead. A value of 0 " > \ > 1992: "(default) disables the native heap trimming.") > \ Nit: s/the// src/hotspot/share/runtime/synchronizer.cpp line 1650: > 1648: > 1649: static size_t delete_monitors(GrowableArray<ObjectMonitor*>* > delete_list) { > 1650: NativeHeapTrimmer::SuspendMark trim_native_pause("monitor deletion"); `sm` will do for the name - as per other uses. src/hotspot/share/runtime/trimNativeHeap.cpp line 83: > 81: > 82: // in seconds > 83: static double now() { return os::elapsedTime(); } Do you need the wrapper for this rather than using `os::elapsedTime()` directly? test/hotspot/gtest/runtime/test_trim_native.cpp line 44: > 42: NativeHeapTrimmer::SuspendMark sm2("Test2"); > 43: { > 44: NativeHeapTrimmer::SuspendMark sm3("Test3"); What is this actually testing? Everything could be a no-op. ------------- PR Review: https://git.openjdk.org/jdk/pull/14781#pullrequestreview-1529587038 PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1263283862 PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1263284320 PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1263285447 PR Review Comment: https://git.openjdk.org/jdk/pull/14781#discussion_r1263292200
