RFR: JDK-8293114: JVM should trim the native heap [v10]

[David Holmes]

On Thu, 13 Jul 2023 18:56:56 GMT, Thomas Stuefe <stuefe at 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