fix(nativemem): count NM_CALLTRACE residency, not reserved chunk capacity - #757
Merged
Conversation
…city
LinearAllocator recorded a whole CALL_TRACE_CHUNK (8 MiB) into NM_CALLTRACE in
allocateChunk(), at the moment the chunk was mmap'd -- before a single byte in
it had been touched. Chunks are then filled incrementally by alloc()'s bump
pointer, and reserveChunk() eagerly reserves the next chunk once the current one
crosses 50% fill, so at any instant at least one fully-counted chunk was mostly
or entirely untouched. The counter moved in 8 MiB steps regardless of real use,
reporting virtual capacity where every other NM_* category reports touched bytes.
Record in alloc() instead, for exactly the bytes handed out. Because the bump
pointer advances monotonically and every returned pointer is written into
immediately by its caller, cumulative bump-allocated bytes track touched --
and therefore resident -- memory directly, with no correction factor.
freeChunk()/freeChunks() decrement each chunk's consumed extent, captured
before safeFree() unmaps it, and clear() un-records the retained _tail's extent
explicitly since no freeChunk() runs for it.
Measured on the dd-trace-doe enterprise workload (12 reps): calltrace falls from
24.23 to 3.53 MiB, a 6.9x over-report. Peak touched bytes never exceed 6.42 MiB,
so this is not a sampling-phase artifact. In an isolated synthetic sweep the
same change moves the counter 24.53 -> 5.38 MiB with every other category
byte-identical, confirming the change is isolated to this one counter.
Cost, isolated allocator microbenchmark (3 runs each, sd < 1 ns):
before after after, peak established
single thread 12.62 ns 25.66 ns 18.15 ns
8 threads contended 64.87 ns 116.3 ns ~78.3 ns
Relative cost roughly doubles, but alloc() is reached only from the
key_value == 0 branch of CallTraceHashTable::put() -- i.e. only for a call trace
not already in the table; a repeat sample of a known stack takes findCallTrace()
and never allocates. So this is a per-distinct-trace cost, not a per-sample one,
putting it in the low milliseconds per 90-second run. About 7.4 ns of the
single-thread increase is record()'s peak high-water CAS, which fires on every
allocation only during pure growth; rotation leaves live below an established
peak, giving the third column.
Also fixes a latent null dereference: allocateChunk() returns NULL on mmap
failure and detachChunks() stores that into _tail, so the destructor's
freeChunk(_tail) could be reached with nothing to free -- previously harmless,
but this change dereferences the chunk to read its extent.
The nativeMem.h header comment described NM_CALLTRACE as an "arena reserved"
figure with CALLTRACE_STORAGE_BYTES as its used slice. Both now count touched
bytes and their small difference is the call-trace hash tables, so computing
arena waste as the difference would yield ~0. Comment corrected.
Known limitation, documented in the code and in
doc/performance/memory-sweep-results-linux.md: bump-allocated bytes are a lower
bound on residency. clear() resets offs and un-records, but does not munmap the
retained _tail, so pages it already touched stay resident while the counter
forgets them. The dominant rotation path (detachChunks -> freeChunks) does
genuinely munmap, bounding this at roughly one chunk. Tracking a per-chunk
touched high-water, kept across clear() and dropped only on real munmap, would
close it.
Tests: linearAllocator_nativemem_ut.cpp pins the new behaviour and fails against
the old code, reporting a whole 1 MiB chunk where 12,800 B were handed out.
Release, debug, ASan and TSan builds all green for
linearAllocator_nativemem_ut, nativeMem_ut, test_callTraceStorage and
stress_callTraceStorage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3160af5ba8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Contributor
CI Test ResultsRun: #33175408346 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-28 13:44:46 UTC |
Contributor
|
❌ 39 passed, 1 failed out of 40 configurations Test Matrix
Failure Detailsmusl-x64-hotspot-jdk25Tracer+profiler: Links
|
Collaborator
|
LGTM! |
jbachorik
approved these changes
Aug 28, 2026
jbachorik
left a comment
jbachorik
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! One small nit only.
Answering review feedback asking whether the _tail dereference needs a guard. A guard at that one line would not have helped: the very next statement (pre-existing) also dereferences _tail, and clear() already dereferences _reserve unguarded at the top of the function. So the hole is the function, not the line. It is reachable: detachChunks() sets both _tail and _reserve to NULL when it cannot allocate a replacement chunk, leaving the allocator deliberately unusable rather than risking a double free. A clear() after that would have faulted on _reserve->prev before ever reaching the _tail read. Early-returns on either pointer being NULL, which covers both dereferences. Complements the same guard already added to freeChunk() for the destructor path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
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
2 participants
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.
What
NM_CALLTRACEreported the virtual capacity of the call-trace arena rather than the memory actually touched, unlike every otherNM_*category. This makes it report residency instead.LinearAllocator::allocateChunk()recorded a whole 8 MiBCALL_TRACE_CHUNKthe moment it wasmmap'd — before any byte in it was touched. Chunks are then filled incrementally byalloc()'s bump pointer, andreserveChunk()eagerly reserves the next chunk once the current crosses 50% fill, so at any instant at least one fully-counted chunk was mostly or entirely untouched.The fix records in
alloc(), for exactly the bytes handed out. Because the bump pointer advances monotonically and every returned pointer is written into immediately by its caller, cumulative bump-allocated bytes track touched — and therefore resident — memory directly, with no correction factor.Why it matters
This counter was feeding a memory-overhead reconciliation that had been closed with a
×0.829residency correction factor borrowed from a different workload. That factor was inflating the call-trace term by ~16 MiB and masking a real, unexplained gap. With the counter measuring directly, the reconciliation is honestly 20–33% under-explained rather than apparently closed — seedoc/performance/dd-trace-doe-reconciliation-2026-08-27.mdonmemory-usage-sweep-handoff.It also reorders the optimisation targets: call-trace was believed to be the dominant category and is actually third, behind
native_symbolsanddictionary.Measurements
dd-trace-doe
enterpriseworkload, 12 reps: calltrace 24.23 → 3.53 MiB, a 6.9× over-report. Peak touched bytes never exceed 6.42 MiB, so this is not a sampling-phase artifact.Isolated synthetic sweep, same build, only the accounting differing:
NM_CALLTRACEEvery other category was byte-identical across that pair, confirming the change is isolated to this counter.
Cost
Isolated allocator microbenchmark, 3 runs each, sd < 1 ns:
Relative cost roughly doubles, but
alloc()is reached only from thekey_value == 0branch ofCallTraceHashTable::put()— only for a call trace not already in the table. A repeat sample of a known stack takesfindCallTrace()and never allocates. So this is a per-distinct-trace cost, not per-sample, which puts it in the low milliseconds per 90-second run.About 7.4 ns of the single-thread increase is
record()'s peak high-water CAS. Its comment notes it fires rarely "since_maxis monotonic" — true for the old once-per-chunk call site, but at a per-alloc call site during pure growth every allocation sets a new high-water. Normal rotation leaves live below an established peak, giving the third column. Coarsening that CAS is a follow-up worth measuring; deliberately not bundled here.Also in this change
allocateChunk()returnsNULLonmmapfailure anddetachChunks()stores that into_tail, so the destructor'sfreeChunk(_tail)could be reached with nothing to free. Previously harmless; this change dereferences the chunk to read its extent, so it now guards.nativeMem.hdescribedNM_CALLTRACEas an "arena reserved" figure withCALLTRACE_STORAGE_BYTESas its used slice. Both now count touched bytes, and their small difference is the call-trace hash tables — so computing arena waste as the difference would yield ~0.Known limitation
Bump-allocated bytes are a lower bound on residency.
clear()resetsoffsand un-records, but does notmunmapthe retained_tail, so pages it already touched stay resident while the counter forgets them. The dominant rotation path (detachChunks→freeChunks) does genuinelymunmap, bounding this at roughly one chunk. Tracking a per-chunk touched high-water — kept acrossclear(), dropped only on realmunmap— would close it. Documented in the code and inmemory-sweep-results-linux.md.Testing
linearAllocator_nativemem_ut.cpp(new) pins the behaviour: byte-accuracy on a fresh chunk, across a chunk boundary with one pre-reserved, and clean return to zero via bothclear()andfreeChunks(). Verified non-vacuous — against the old code it fails, reporting a whole 1 MiB chunk where 12,800 B had been handed out.Release, debug, ASan and TSan all green for
linearAllocator_nativemem_ut,nativeMem_ut,test_callTraceStorage,stress_callTraceStorage. The sanitizer runs matter here because the change adds a read ofcurrent->offson the free path; TSan confirms no race and ASan no use-after-free.🤖 Generated with Claude Code