From 3160af5ba81b931dcbd285527c74d73bd085292a Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 27 Aug 2026 12:26:39 +0000 Subject: [PATCH 1/2] fix(nativemem): count NM_CALLTRACE residency, not reserved chunk capacity 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) --- ddprof-lib/src/main/cpp/linearAllocator.cpp | 39 ++++++- ddprof-lib/src/main/cpp/nativeMem.h | 15 ++- .../test/cpp/linearAllocator_nativemem_ut.cpp | 101 ++++++++++++++++++ 3 files changed, 146 insertions(+), 9 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp diff --git a/ddprof-lib/src/main/cpp/linearAllocator.cpp b/ddprof-lib/src/main/cpp/linearAllocator.cpp index cc12da403c..c6359055a8 100644 --- a/ddprof-lib/src/main/cpp/linearAllocator.cpp +++ b/ddprof-lib/src/main/cpp/linearAllocator.cpp @@ -102,6 +102,10 @@ void LinearAllocator::clear() { _reserve = current; _tail = current; } + // _tail is kept (not freed) but its contents are discarded here, so its + // consumed bytes must be un-recorded explicitly -- freeChunk() won't run + // for this one. + NativeMem::record(NM_CALLTRACE, -(long long)(_tail->offs - sizeof(Chunk))); _tail->offs = sizeof(Chunk); // DON'T UNPOISON HERE - let alloc() do it on-demand! @@ -162,15 +166,17 @@ void LinearAllocator::freeChunks(ChunkList& chunks) { __tsan_acquire(current); #endif Chunk* prev = current->prev; + // Capture before safeFree unmaps the chunk -- reading current->offs after + // that would touch freed memory. This is exactly the byte count alloc() + // recorded into NM_CALLTRACE for this chunk, so the decrement is exact. + long long used = (long long)(current->offs - sizeof(Chunk)); #ifdef TSAN_ENABLED __tsan_release(current); #endif OS::safeFree(current, chunks.chunk_size); Counters::decrement(LINEAR_ALLOCATOR_BYTES, chunks.chunk_size); Counters::decrement(LINEAR_ALLOCATOR_CHUNKS); - // The LinearAllocator's only user is call-trace storage, so all of its - // chunk memory is attributed to the CALLTRACE category. - NativeMem::record(NM_CALLTRACE, -(long long)chunks.chunk_size); + NativeMem::record(NM_CALLTRACE, -used); current = prev; } @@ -195,6 +201,11 @@ void *LinearAllocator::alloc(size_t size) { if (__sync_bool_compare_and_swap(&chunk->offs, offs, offs + size)) { void* allocated_ptr = (char *)chunk + offs; + // The LinearAllocator's only user is call-trace storage, so all of + // its bump-allocated bytes are attributed to the CALLTRACE category. + // A relaxed atomic add, safe to call from the sampling signal handler. + NativeMem::record(NM_CALLTRACE, (long long)size); + // ASAN UNPOISONING: Unpoison ONLY the allocated region on-demand // This allows ASan to detect use-after-free of memory that was cleared // but not yet reallocated @@ -264,12 +275,30 @@ Chunk *LinearAllocator::allocateChunk(Chunk *current) { Counters::increment(LINEAR_ALLOCATOR_BYTES, _chunk_size); Counters::increment(LINEAR_ALLOCATOR_CHUNKS); - NativeMem::record(NM_CALLTRACE, (long long)_chunk_size); + // NM_CALLTRACE is NOT recorded here. Recording the full chunk size at + // reservation time would count virtual capacity, not residency: the + // chunk is mmap'd whole but filled incrementally by alloc()'s bump + // pointer, and reserveChunk() eagerly reserves the next chunk at 50% + // fill of the current one, so there is always at least one chunk that's + // fully counted but mostly or entirely untouched. alloc() instead + // records exactly the bytes actually handed out, which -- because the + // bump pointer advances linearly and every returned pointer is written + // into immediately by the caller -- tracks touched (resident) bytes + // directly, with no separate residency measurement needed. } return chunk; } void LinearAllocator::freeChunk(Chunk *current) { + // allocateChunk() returns NULL when the mmap fails, and detachChunks() stores + // that NULL into _tail, so the destructor's freeChunk(_tail) can be reached + // with nothing to free. Bail out rather than dereference it below. + if (current == NULL) { + return; + } + // Capture before safeFree unmaps the chunk -- see freeChunks() for why this + // exactly reverses what alloc() recorded for this chunk. + long long used = (long long)(current->offs - sizeof(Chunk)); // Release TSan ownership before munmap so the sanitizer knows this thread is // done with the memory. The mmap(MAP_FIXED) re-map in allocateChunk() resets // the shadow for whichever thread later reuses this VA (after OS VA reuse), so @@ -280,7 +309,7 @@ void LinearAllocator::freeChunk(Chunk *current) { OS::safeFree(current, _chunk_size); Counters::decrement(LINEAR_ALLOCATOR_BYTES, _chunk_size); Counters::decrement(LINEAR_ALLOCATOR_CHUNKS); - NativeMem::record(NM_CALLTRACE, -(long long)_chunk_size); + NativeMem::record(NM_CALLTRACE, -used); } void LinearAllocator::reserveChunk(Chunk *current) { diff --git a/ddprof-lib/src/main/cpp/nativeMem.h b/ddprof-lib/src/main/cpp/nativeMem.h index 2706f56297..fc89449f4b 100644 --- a/ddprof-lib/src/main/cpp/nativeMem.h +++ b/ddprof-lib/src/main/cpp/nativeMem.h @@ -13,10 +13,17 @@ // per-category live byte gauges partition the total: sum(category) == total, // with no double counting. // -// The "reserved vs used vs wasted" breakdowns exposed by the existing counters -// (CALLTRACE_STORAGE_BYTES is the used slice of the CALLTRACE arena; -// DICTIONARY_ARENA_WASTE_BYTES is the wasted slice of the DICTIONARY arena) are -// a separate, nested dimension. They are intentionally NOT summed in here. +// These gauges track memory that is actually touched (and therefore resident), +// not address space that has merely been reserved. NM_CALLTRACE in particular +// counts the bytes LinearAllocator::alloc() has bump-allocated, NOT the +// capacity of the 8 MiB chunks backing them -- see linearAllocator.cpp. +// +// Consequently NM_CALLTRACE is no longer 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 (also bump-allocated), +// not chunk slack. Do not compute arena waste as the difference between them. +// DICTIONARY_ARENA_WASTE_BYTES remains a genuine nested waste figure for the +// DICTIONARY arena. None of these nested counters are summed in here. #define DD_NATIVE_MEM_CATEGORY_TABLE(X) \ X(CALLTRACE, "calltrace") \ X(DICTIONARY, "dictionary") \ diff --git a/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp b/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp new file mode 100644 index 0000000000..8a0299127a --- /dev/null +++ b/ddprof-lib/src/test/cpp/linearAllocator_nativemem_ut.cpp @@ -0,0 +1,101 @@ +#include "gtest/gtest.h" +#include "linearAllocator.h" +#include "nativeMem.h" + +// NM_CALLTRACE must track bytes actually handed out by alloc() -- i.e. touched, +// resident bytes -- not the virtual capacity of the mmap'd chunks backing them. +// The distinction is large: chunks are 8 MiB (CALL_TRACE_CHUNK) and alloc() +// eagerly reserves the next one at 50% fill, so a capacity-based counter +// over-reports by up to two chunks' worth of untouched address space. + +static const size_t CHUNK_SIZE = 1024 * 1024; + +class LinearAllocatorNativeMemTest : public ::testing::Test { +protected: + long long _baseline[NM_NUM_CATEGORIES]; + + // Static-duration objects elsewhere in the binary record into these same + // categories before main() runs; restore their baseline in TearDown so + // their destructors don't underflow a category this fixture zeroed. + // (Same rationale as NativeMemTest in nativeMem_ut.cpp.) + void SetUp() override { + for (int c = 0; c < NM_NUM_CATEGORIES; c++) { + _baseline[c] = NativeMem::live((NativeMemCategory)c); + } + NativeMem::reset(); + } + void TearDown() override { + NativeMem::reset(); + for (int c = 0; c < NM_NUM_CATEGORIES; c++) { + NativeMem::setLive((NativeMemCategory)c, _baseline[c]); + } + } +}; + +TEST_F(LinearAllocatorNativeMemTest, ReportsBumpedBytesNotChunkCapacity) { + const size_t ALLOC_SIZE = 128; + const int NUM_ALLOCS = 100; + + LinearAllocator allocator(CHUNK_SIZE); + // Constructing the allocator mmaps a chunk but touches nothing in it. + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "a freshly reserved chunk is untouched and must not be counted"; + + for (int i = 0; i < NUM_ALLOCS; i++) { + ASSERT_NE(nullptr, allocator.alloc(ALLOC_SIZE)); + } + + EXPECT_EQ((long long)(ALLOC_SIZE * NUM_ALLOCS), NativeMem::live(NM_CALLTRACE)) + << "live bytes must equal exactly the bytes alloc() handed out"; + EXPECT_LT(NativeMem::live(NM_CALLTRACE), (long long)CHUNK_SIZE) + << "must not have jumped to whole-chunk granularity"; +} + +TEST_F(LinearAllocatorNativeMemTest, CrossingIntoASecondChunkStaysByteAccurate) { + // An allocation size that does not divide the chunk evenly, driven past the + // point where reserveChunk() pre-reserves the next chunk, so the reserved + // chunk's untouched capacity would show up in a capacity-based counter. + const size_t ALLOC_SIZE = 3000; + const int NUM_ALLOCS = (int)(CHUNK_SIZE / ALLOC_SIZE) + 50; + + LinearAllocator allocator(CHUNK_SIZE); + long long handed_out = 0; + for (int i = 0; i < NUM_ALLOCS; i++) { + if (allocator.alloc(ALLOC_SIZE) != nullptr) { + handed_out += (long long)ALLOC_SIZE; + } + } + + EXPECT_EQ(handed_out, NativeMem::live(NM_CALLTRACE)) + << "byte-accurate across a chunk boundary, with a chunk pre-reserved"; +} + +TEST_F(LinearAllocatorNativeMemTest, ClearReturnsLiveToZero) { + LinearAllocator allocator(CHUNK_SIZE); + for (int i = 0; i < 50; i++) { + ASSERT_NE(nullptr, allocator.alloc(256)); + } + ASSERT_GT(NativeMem::live(NM_CALLTRACE), 0); + + allocator.clear(); + + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "clear() discards all chunk contents, including the retained _tail's"; +} + +TEST_F(LinearAllocatorNativeMemTest, FreeChunksReturnsLiveToZero) { + LinearAllocator allocator(CHUNK_SIZE); + for (int i = 0; i < 50; i++) { + ASSERT_NE(nullptr, allocator.alloc(256)); + } + long long before_detach = NativeMem::live(NM_CALLTRACE); + ASSERT_GT(before_detach, 0); + + ChunkList detached = allocator.detachChunks(); + EXPECT_EQ(before_detach, NativeMem::live(NM_CALLTRACE)) + << "detaching moves ownership but frees nothing yet"; + + LinearAllocator::freeChunks(detached); + EXPECT_EQ(0, NativeMem::live(NM_CALLTRACE)) + << "freeing the detached chunks un-records exactly what alloc() recorded"; +} From 50c92bb8357cf1b6166a04c33523caac696c3038 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Fri, 28 Aug 2026 12:14:38 +0000 Subject: [PATCH 2/2] fix(nativemem): make LinearAllocator::clear() null-safe 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) --- ddprof-lib/src/main/cpp/linearAllocator.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ddprof-lib/src/main/cpp/linearAllocator.cpp b/ddprof-lib/src/main/cpp/linearAllocator.cpp index c6359055a8..22ea167068 100644 --- a/ddprof-lib/src/main/cpp/linearAllocator.cpp +++ b/ddprof-lib/src/main/cpp/linearAllocator.cpp @@ -46,6 +46,14 @@ LinearAllocator::~LinearAllocator() { } void LinearAllocator::clear() { + // Both pointers are NULL only after detachChunks() could not allocate a + // replacement chunk, which leaves the allocator deliberately unusable rather + // than risking a double free. Bail out here so the whole function is + // null-safe: it dereferences _reserve immediately below and _tail at the end, + // so guarding either one alone would leave the other exposed. + if (_tail == NULL || _reserve == NULL) { + return; + } // OS::safeAlloc/safeFree use raw syscalls not intercepted by TSan, so TSan // never clears shadow memory on munmap. Add explicit acquire/release around // every plain prev-field read so the happens-before chain from freeChunk's