Explicitly trim the heap after cache sweeps - #6022
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6022 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 846 848 +2
Lines 67746 67763 +17
Branches 7560 7558 -2
=========================================
+ Hits 54067 54072 +5
- Misses 13679 13691 +12
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| auto const statusBefore = readFile(statusPath); | ||
| report.rssBeforeKB = detail::parseVmRSSkB(statusBefore); | ||
|
|
||
| report.trimResult = ::malloc_trim(0); |
There was a problem hiding this comment.
I would suggest, instead of calling malloc_trim(0), which would try to trim up to the main heap's top boundary (leaving only a minimum amount, 1 page, which is usually 4 or 8 KB, link for other readers), we should instead call malloc_trim(m);, where m is the minimum amount of memory we expect we will need soon enough. It could range from few hundred MBs to few GBs. The description of this PR mentions a rate of about ~1.2 GB/h (on the higher side). So, I would suggest we keep m=~2.4GB for starters. The reports we are accumulating in this class can then help us fine tune it.
Why?
Memory allocation is an expensive operation in itself. In this case(after calling malloc_trim(0)) any new allocations would also require heap extension. This will be exceptionally expensive(by the order of 1000 times, involving user to kernel space context switch and then allocation of new pages by OS). We can avoid that by keeping few GBs in reserve.
The trim after NetworkOps -> SyncComplete could be one of the suboptimal calls, if we do any heavy operations after SyncComplete, requiring memory allocations exceeding 1 page.
There was a problem hiding this comment.
This note in the manual suggests that attempting to use the trim padding is wasted effort:
Only the main heap (using sbrk(2)) honors the pad argument; thread heaps do not.
There was a problem hiding this comment.
A malloc_trim(m) call will check all the arenas (main and thread) to see if chunks of memory can be released. It will use the padding arg to decide(keep upto) the chunk that can be freed from main heap top.
For thread heaps(sub-heaps), malloc_trim anyway can't free the sub-heap or part of it(hence neglect padding), if there's even a small block in use. If a sub-heap region becomes completely empty after the last call to free(), allocator will anyway return the memory to OS. So padding doesn't make much sense for thread heaps. Thread heaps are anyway self-managed. So, we don't need to optimise for them. If there's fragmentation in the sub-heaps, that will remain until an entire sub-heap is empty(which will then be released).
There was a problem hiding this comment.
The trim after NetworkOps -> SyncComplete could be one of the suboptimal calls, if we do any heavy operations after SyncComplete, requiring memory allocations exceeding 1 page.
I revisited the malloc_trim triggers. The sweep-based trigger accounts for the largest observed memory reclamation (as shown in the candle charts). Piggybacking on sweep provides regular reclamation from a background job context, keeping trim overhead away from consensus/RPC hot paths. As a result, I left only the sweep-based trigger.
There was a problem hiding this comment.
Also, I did research exploring padding and ran several tests.
1) Memory reclaim
| Padding | Mean RSS after 1h (MB) | P95 RSS after 1h (MB) | End RSS (MB) | Notes |
|---|---|---|---|---|
| 0 | 16013.7 | 16611.0 | 16579.5 | Best sustained/typical RSS |
| 256KB | 16288.9 | 16900.6 | 16729.5 | Slightly higher RSS than 0 |
| 1MB | 16397.9 | 17052.5 | 15877.5 | Lowest final RSS, but higher average/variance |
| 16MB | 16151.4 | 17120.1 | 17360.6 | Highest end RSS; retains most memory late-run |
Interpretation
- If we care about consistent memory footprint, 0 padding looks best (lowest mean and p95 RSS after warm-up).
- 1MB appears to allow memory to build up more during the run, then occasionally drops sharply (hence low end RSS but higher average).
- 16MB keeps a larger “reserve” in the allocator, and the run ends with the highest RSS.
2) Latency impact
| Padding | Calls | Avg (ms) | P95 (ms) | P99 (ms) | Max (ms) |
|---|---|---|---|---|---|
| 0 | 670 | 162.9 | 392.5 | 673.6 | 904.4 |
| 256KB | 706 | 171.0 | 372.6 | 702.2 | 837.1 |
| 1MB | 645 | 178.6 | 415.8 | 794.5 | 989.0 |
| 16MB | 621 | 166.2 | 372.1 | 673.4 | 929.3 |
Interpretation
- 1MB is the clear latency outlier (highest avg / p95 / p99 / max).
- 256KB shows slightly better p95 and max than 0, but slightly worse average.
- 16MB doesn’t materially improve tail latency vs 256KB, and still has a high max.
Based on the 12‑hour Mainnet runs, there isn’t a strong, consistent benefit to moving away from the default malloc_trim behavior (padding = 0). While small non‑zero paddings (e.g., 256KB) can marginally shift some tail-latency metrics, the improvements are not decisive and come with trade-offs (slightly higher RSS or no clear sustained memory advantage). Larger paddings (1MB and 16MB) do not provide a compelling overall benefit: 1MB showed worse trim-latency characteristics, and 16MB tended to retain more memory without meaningful latency wins.
We should keep padding = 0 (default) as the production setting. It is the simplest, most predictable choice and provides a solid balance of memory reclamation and trim overhead without introducing additional tuning surface area or risk of unexpected behavior under varying Mainnet load.
There was a problem hiding this comment.
So, as per our discussion today, I understand that the memory we are claiming in each call to malloc_trim is in MBs. So, using a padding of few hundred MBs won't give us much trimming. Also, there's some latency introduced by each reclaim call. So, we don't want to wait too long, and claim a bigger chunk, since it will take some time and might introduce some slowness in the app.
So, the current implementation looks fine to me. I would be interested in seeing how it performs on prod. and then we can fine tune the frequency and the padding params further.
lmaisons
left a comment
lmaisons
left a comment
There was a problem hiding this comment.
The points where we trim look reasonable. Unless there's some obvious caveat I'm not aware of, I think the RSS reporting should use the self and statm proc features so as to minimize parsing / clerical errors.
|
|
||
| std::string const tagStr = tag.value_or("default"); | ||
| std::string const statusPath = | ||
| "/proc/" + std::to_string(cachedPid) + "/status"; |
There was a problem hiding this comment.
Could we not just use /proc/self/...? Also, it seems we would need to do fewer parsing contortions if we read from /proc/self/statm instead of /proc/self/status
There was a problem hiding this comment.
Thanks! I agree, it simplifies this part of the code a lot.
| auto const statusBefore = readFile(statusPath); | ||
| report.rssBeforeKB = detail::parseVmRSSkB(statusBefore); | ||
|
|
||
| report.trimResult = ::malloc_trim(0); |
There was a problem hiding this comment.
This note in the manual suggests that attempting to use the trim padding is wasted effort:
Only the main heap (using sbrk(2)) honors the pad argument; thread heaps do not.
My thoughts exactly. See my reply to @a1q123456's comment above |
vlntb
added
the
Ready to merge
There was a problem hiding this comment.
Pull request overview
This PR introduces explicit heap memory trimming functionality to address memory growth issues in long-running rippled nodes on Linux/glibc systems. The implementation adds a new MallocTrim utility in libxrpl that wraps malloc_trim(0) and integrates it into the application's periodic sweep operations.
Changes:
- Adds
MallocTrimhelper with RSS tracking and diagnostic reporting - Integrates
mallocTrim()call intoApplication::doSweep()after cache sweeps - Provides comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| include/xrpl/basics/MallocTrim.h | Defines MallocTrimReport struct and mallocTrim() function with platform-specific documentation |
| src/libxrpl/basics/MallocTrim.cpp | Implements malloc_trim wrapper with RSS measurement via /proc/self/statm and rusage tracking |
| src/tests/libxrpl/basics/MallocTrim.cpp | Comprehensive unit tests covering report structure, parsing, and trim functionality across platforms |
| src/xrpld/app/main/Application.cpp | Integrates mallocTrim call at end of doSweep() after all cache cleanup operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vlntb
removed
the
Ready to merge
ximinez
changed the title
Limited to Linux/glibc builds.
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
High Level Overview of Change
MallocTrimhelper in libxrpl to centralize calls to::malloc_trim(0)on Linux/glibc, and (optionally) record RSS before/after for debugging and reporting.Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)