RFR: JDK-8293114: GC should trim the native heap [v11]
Aleksey Shipilev
shade at openjdk.org
Wed Jul 5 18:45:18 UTC 2023
On Wed, 5 Jul 2023 17:28:15 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> (*Updated 2023-07-05 to reflect the current state of the patch*)
>>
>> This RFE adds the option to auto-trim the Glibc heap as part of the GC cycle. If the VM process suffered high temporary malloc spikes (regardless of whether from JVM- or user code), this could recover significant amounts of memory.
>>
>> We discussed this a year ago [1], but the item got pushed to the bottom of my work pile, therefore, it took longer than I thought.
>>
>> ### Motivation
>>
>> The Glibc is reluctant to return memory to the OS, more so than other allocators. 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, and a performance tradeoff by the glibc. It makes a lot of sense with applications that cause high traffic on the C-heap (the typical native application). The JVM, however, clusters allocations and for a lot of use cases rolls its own memory management via mmap. And app's malloc load can fluctuate wildly, with temporary spikes and long idle periods.
>>
>> To help, Glibc exports an API to trim the C-heap: `malloc_trim(3)`. With JDK 18 [2], SAP contributed a new jcmd command to *manually* trim the C-heap on Linux. This RFE adds a complementary way to trim automatically.
>>
>> #### Is this even a problem?
>>
>> Yes.
>>
>> The JVM clusters most native memory allocations and satisfies them with mmap. But there are enough C-heap allocations left to cause malloc spikes that are subject of memory retention. Note that one example are hotspot arenas themselves.
>>
>> But many cases of high memory retention in Glibc I have seen in third-party JNI code. Libraries allocate large buffers via malloc as temporary buffers. In fact, since we introduced the jcmd "System.trim_native_heap", some of our customers started to call this command periodically in scripts to counter these issues.
>>
>> ### How trimming works
>>
>> Trimming is done via `malloc_trim(2)`. `malloc_trim` will iterate over all arenas and trim each one subsequently. While doing that, it will lock the arena, which may cause some (but not all) subsequent actions on the same arenas to block. glibc also trims automatically on free, but that is very limited (see https://github.com/openjdk/jdk/pull/10085#issuecomment-1619638641 for details).
>>
>> `malloc_trim` offers almost no way to control its behavior; in particular, no way to limit its runtime. Its run...
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 41 commits:
>
> - fix windows build
> - Merge branch 'master' into JDK-8293114-GC-trim-native
> - wip
> - Merge branch 'master' into JDK-8293114-GC-trim-native
> - wip
> - Remove adaptive stepdown coding
> - Merge master
> - wip
> - Merge branch 'master' into JDK-8293114-GC-trim-native
> - wip
> - ... and 31 more: https://git.openjdk.org/jdk/compare/22e17c29...162b880a
Cursory review follows.
Generally, does the issue synopsis reflect what is going on correctly? This is not about "GC should trim" anymore, as we have a perfectly separate thread for this. In fact, we very specifically _do not_ trim during GC :)
Related question if we want to use `gc, trim` tag for this, or just `trim`.
src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 918:
> 916: }
> 917:
> 918: TrimNative::PauseMark trim_native_pause("gc");
Maybe instead of whack-a-mole game of putting the pause in most GC safepoint ops, we should "just" put this mark straight into `VMOperation`, so that _all_ safepoint ops are covered? Or, if we want to limit to GC ops, `VM_GC_Operation`?
src/hotspot/share/gc/g1/g1FullCollector.cpp line 181:
> 179:
> 180: void G1FullCollector::prepare_collection() {
> 181:
Redundant.
src/hotspot/share/gc/shared/gc_globals.hpp line 696:
> 694: \
> 695: product(bool, TrimNativeHeap, false, EXPERIMENTAL, \
> 696: "GC will attempt to trim the native heap periodically. By " \
This is not trimmed by GC anymore, right? By a separate thread now?
I would also avoid mentioning the default here, just refer to TrimNativeHeapInterval. Saves the headache when default actually changes.
src/hotspot/share/gc/shared/gc_globals.hpp line 700:
> 698: "changed using TrimNativeHeapInterval.") \
> 699: \
> 700: product(uint, TrimNativeHeapInterval, 60, EXPERIMENTAL, \
Most intervals like these are in milliseconds in Hotspot. Actually, I think it would be useful to have the sub-second interval for some short-lived workloads.
src/hotspot/share/gc/shared/trimNative.cpp line 40:
> 38: class NativeTrimmerThread : public ConcurrentGCThread {
> 39:
> 40: Monitor* _lock;
`const`?
src/hotspot/share/gc/shared/trimNative.cpp line 104:
> 102: void execute_trim_and_log() const {
> 103: assert(os::can_trim_native_heap(), "Unexpected");
> 104: const int64_t tnow = now();
This line looks redundant.
src/hotspot/share/gc/shared/trimNative.hpp line 46:
> 44: // Pause periodic trimming while in scope; when leaving scope,
> 45: // resume periodic trimming.
> 46: struct PauseMark {
Maybe move it out? Looks a bit weird to see `TrimNative::PauseMark` instead of e.g. `TrimNativePauseMark`.
Also "pause" might be confusing with GC. Maybe `TrimNativeSuspend` or even `NativeTrimSuspend`?
src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 288:
> 286: heap->pacer()->setup_for_idle();
> 287: }
> 288:
Redundant.
src/hotspot/share/gc/shenandoah/shenandoahPhaseTimings.hpp line 177:
> 175: \
> 176: f(conc_uncommit, "Concurrent Uncommit") \
> 177: f(conc_trim, "Concurrent Trim") \
Looks like a leftover.
src/hotspot/share/memory/arena.cpp line 101:
> 99: STATIC_ASSERT(_num_pools == 4);
> 100: return !_pools[0].empty() || !_pools[1].empty() ||
> 101: !_pools[2].empty() || !_pools[3].empty();
Suggestion:
for (int i = 0; i < _num_pools; i++) {
if (!_pools[i].empty()) return false;
}
return true;
src/hotspot/share/memory/arena.cpp line 105:
> 103:
> 104: static void clean() {
> 105: ThreadCritical tc;
Why do you need `ThreadCritical` here? I would have thought `PauseMark` handles everything right?
-------------
PR Review: https://git.openjdk.org/jdk/pull/10085#pullrequestreview-1120265106
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253439204
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253435572
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253445627
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253447207
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253483365
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253490281
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253482841
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253471989
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253486311
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253486975
PR Review Comment: https://git.openjdk.org/jdk/pull/10085#discussion_r1253488630
More information about the hotspot-gc-dev
mailing list