perf(rmw-zenoh-rs): pin glibc trim threshold at init - #350
Merged
Merged
Conversation
…rence Justify M_MMAP_THRESHOLD explicitly (pinning M_TRIM_THRESHOLD alone freezes it at 128 KiB, routing every payload through mmap -- measured worse than not touching either), state the 4 MiB figure as measured rather than a general glibc guarantee, and explain the 64 MiB bound against glibc's own threshold-adaptation ceiling.
mallopt() failure previously logged at debug, so a silently-rejected pin would look identical to the fix simply not applying -- exactly the symptom this call exists to prevent. Elevate to warn on failure only; the success path stays at debug.
There was a problem hiding this comment.
🟡 Changes recommended
Allocator status is logged before tracing initialization, making failures silent on the normal path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Pins glibc allocator thresholds during RMW initialization to reduce large-message latency and page faults.
Changes:
- Sets mmap and trim thresholds to 64 MiB.
- Preserves operator-provided allocator settings.
- Reports
malloptsuccess or failure through tracing.
File summaries
| File | Description |
|---|---|
crates/rmw-zenoh-rs/src/context.rs |
Configures glibc allocation policy during rmw_init. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The mallopt block ran before zenoh::init_log_from_env_or installed the tracing subscriber, so its warn!/debug! calls fired into a void and were silently dropped -- exactly the visibility the warn-on-failure change was meant to add. Move the block after logging init. Also correct the comment's claim that both mallopt values sit "inside glibc's own envelope": glibc's dynamic M_MMAP_THRESHOLD adjustment caps at ~32 MiB (DEFAULT_MMAP_THRESHOLD_MAX), so pinning it to 64 MiB is a deliberate override beyond what glibc would choose on its own for that parameter. M_TRIM_THRESHOLD is the one whose dynamic ceiling is 64 MiB (twice the mmap threshold), so the two do not share one envelope.
Cites #349 for the mechanism and measurements instead of restating them at the call site -- brk/fault counts and the 6-9 MB working-set figure were exact duplicates of the issue and would drift independently of it. Keeps the one number worth having locally (33% latency cost) and every fact a reader needs to avoid breaking the fix (both calls required together, why 64 MiB, the operator opt-out). 39 lines -> 20.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rmw_zenoh_rsallocates a fresh payload buffer per message. At 1 MiB this leaves glibc's heap-trim threshold below the process's working set. glibc hands the heap back withbrk, and the next message re-faults it. This costs 33% of round-trip latency. It also makes the cell bimodal between runs. Fixes #349.Key Changes
This PR adds two
malloptcalls atrmw_init. They pinM_MMAP_THRESHOLDandM_TRIM_THRESHOLDto 64 MiB. This fix skips both calls when the operator has already set the correspondingMALLOC_*environment variables.This fix needs both calls together.
mallopton either threshold disables glibc's dynamic adjustment of both of them. Pinning one alone leaves the other frozen at its 128 KiB default. That measures worse than not touching either threshold (see #349 for the lever-separation data).This PR checks
mallopt's return code. On failure, it logs atwarn. On success, it logs atdebug. Otherwise, a silently-rejected pin would look identical to the fix simply not applying.What fails without this
No test goes red. This is a performance defect. The failing baseline is the measurement in #349: 4700 us vs 3137 us at 1 MiB / 200 Hz, against the pinned-environment-variable target. The rep-to-rep spread falls from 45% to 6% against that same target. This PR's change reproduces that target without requiring the operator to set anything.
Test coverage
None added. No existing test exercises
rmw_init, with or without this change. That gap pre-dates this PR.A meaningful test of the core effect would need to assert glibc's internal allocator state. That is not practical from a Rust unit test. The opt-out logic is testable in principle: it skips the pin when
MALLOC_*is already set. But that logic is currently inlined inrmw_init, not factored into a separate function. So this PR adds no test for it.This PR states the gap explicitly. A green CI run does not imply behavioural coverage that does not exist.
Breaking Changes
None functionally. Disclosed for completeness: this change sets glibc allocator policy for the whole process, not just for this crate's own allocations. It skips doing so if the operator has already set
MALLOC_TRIM_THRESHOLD_orMALLOC_MMAP_THRESHOLD_. An explicit deployment choice is never overridden.