From cf42bb101900e542a11975befba2ad446ee2be97 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 21 Jul 2026 13:03:29 +0200 Subject: [PATCH 1/3] fix(codecache): make memoryUsage() accurate and live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeCache::memoryUsage() drove the CODECACHE_NATIVE_SIZE_BYTES counter but both under-counted and went stale: - The blob array was counted as _capacity * sizeof(CodeBlob*) — a pointer's worth — while the array holds CodeBlob (three pointers), a ~3x undercount. - Symbol name strings were approximated as _count * sizeof(NativeFunc), a fixed 4 bytes each, ignoring the name length that dominates the actual allocation. - The DWARF unwind table and build-id string were not counted at all. - CodeCacheArray cached the sum at add() time, so it never reflected a library's later symbol growth. Recompute both accurately on demand instead. CodeCache::memoryUsage() now sums the full blob array, each symbol's real name-string allocation (via new NativeFunc::allocSize()), its own name, the DWARF table, and the build-id. CodeCacheArray::memoryUsage() sums the live per-library values (the array is append-only, so iterating the published prefix is safe alongside concurrent adds); the stale _used_memory cache is removed. memoryUsage() is only called at dump time, so the O(symbols) recompute is not on any hot path. Tests: new codeCache_ut asserts usage tracks per-symbol name length, that a longer name costs more than a shorter one, and that the blob array is counted at full CodeBlob size rather than a pointer's. Co-Authored-By: Claude Opus 4.8 (1M context) --- ddprof-lib/src/main/cpp/codeCache.cpp | 22 +++++++++++ ddprof-lib/src/main/cpp/codeCache.h | 35 +++++++++++++---- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 49 ++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 7 deletions(-) create mode 100644 ddprof-lib/src/test/cpp/codeCache_ut.cpp diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 5203d5c5d2..d460e5f6b0 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -144,6 +144,28 @@ CodeCache::~CodeCache() { free(_build_id); // Free build-id memory } +long long CodeCache::memoryUsage() { + // Blob array. The previous formula used sizeof(CodeBlob*), undercounting the + // array roughly threefold (CodeBlob holds three pointers). + long long total = (long long)_capacity * sizeof(CodeBlob); + + // This cache's own name, plus each symbol's variable-length name string. The + // previous formula approximated these as a fixed sizeof(NativeFunc), ignoring + // the name length that dominates the allocation. + total += (long long)NativeFunc::allocSize(_name); + for (int i = 0; i < _count; i++) { + total += (long long)NativeFunc::allocSize(_blobs[i]._name); + } + + // DWARF unwind table and build-id string were not counted at all before. + total += (long long)_dwarf_table_length * sizeof(FrameDesc); + if (_build_id != nullptr) { + total += (long long)strlen(_build_id) + 1; + } + + return total; +} + void CodeCache::expand() { CodeBlob *old_blobs = _blobs; CodeBlob *new_blobs = new CodeBlob[_capacity * 2]; diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 92b45bf472..d6ef7955a2 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -74,6 +74,15 @@ class NativeFunc { static char *create(const char *name, short lib_index); static void destroy(char *name); + // Size of the heap allocation backing a name string produced by create(). + // Mirrors the size computed there so callers can account for it. 0 if null. + static size_t allocSize(const char *name) { + if (name == nullptr) { + return 0; + } + return align_up(sizeof(NativeFunc) + 1 + strlen(name), sizeof(NativeFunc *)); + } + static short libIndex(const char *name) { if (name == nullptr) { return -1; @@ -251,9 +260,11 @@ class CodeCache { void setDwarfTable(FrameDesc *table, int length, const FrameDesc &default_frame = FrameDesc::default_frame); FrameDesc findFrameDesc(const void *pc); - long long memoryUsage() { - return _capacity * sizeof(CodeBlob *) + _count * sizeof(NativeFunc); - } + // Accurate live size of everything this CodeCache owns on the heap: the blob + // array, the per-symbol name strings (variable length), the DWARF unwind + // table, the build-id string, and this cache's own name. Recomputed on + // demand (called only at dump time), so it always reflects current contents. + long long memoryUsage(); int count() { return _count; } CodeBlob* blob(int idx) { @@ -266,11 +277,10 @@ class CodeCacheArray { CodeCache *_libs[MAX_NATIVE_LIBS]; volatile int _reserved; // next slot to reserve (CAS by writers) volatile int _count; // published count (all indices < _count have non-NULL pointers) - volatile size_t _used_memory; bool _overflow_reported; public: - CodeCacheArray() : _reserved(0), _count(0), _used_memory(0), _overflow_reported(false) { + CodeCacheArray() : _reserved(0), _count(0), _overflow_reported(false) { memset(_libs, 0, MAX_NATIVE_LIBS * sizeof(CodeCache *)); } @@ -296,7 +306,6 @@ class CodeCacheArray { } while (!__atomic_compare_exchange_n(&_reserved, &slot, slot + 1, true, __ATOMIC_RELAXED, __ATOMIC_RELAXED)); assert(__atomic_load_n(&_libs[slot], __ATOMIC_RELAXED) == nullptr); - __atomic_fetch_add(&_used_memory, lib->memoryUsage(), __ATOMIC_RELAXED); // Store pointer before publishing count. The RELEASE here pairs with // the ACQUIRE load in operator[]/at() and count(). __atomic_store_n(&_libs[slot], lib, __ATOMIC_RELEASE); @@ -316,8 +325,20 @@ class CodeCacheArray { return __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE); } + // Sum the live memory of all registered libraries. Recomputed on demand + // (called only at dump time) so it tracks each library's later growth, + // unlike a value cached at add() time. The array is append-only, so + // iterating the published prefix is safe alongside concurrent add()s. size_t memoryUsage() const { - return __atomic_load_n(&_used_memory, __ATOMIC_RELAXED); + size_t total = 0; + int n = count(); + for (int i = 0; i < n; i++) { + CodeCache *lib = at(i); + if (lib != nullptr) { + total += (size_t)lib->memoryUsage(); + } + } + return total; } }; diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp new file mode 100644 index 0000000000..039de04b0f --- /dev/null +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -0,0 +1,49 @@ +/* + * Copyright 2026, Datadog, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#include "codeCache.h" +#include + +// memoryUsage() must reflect the actual per-symbol name length, not a fixed +// per-symbol constant: adding one symbol grows the reported usage by exactly +// the allocation size of its name string. +TEST(CodeCacheTest, MemoryUsageCountsSymbolNameLength) { + CodeCache cc("testlib"); + long long base = cc.memoryUsage(); + + const char *name = + "a_long_symbol_name_well_beyond_sizeof_NativeFunc_padding_xxxxxxxxxxxx"; + cc.add((const void *)0x1000, 16, name); + + EXPECT_EQ(NativeFunc::allocSize(name), (size_t)(cc.memoryUsage() - base)); +} + +// A longer name costs more than a shorter one — i.e. length actually matters, +// which the old fixed sizeof(NativeFunc) formula could not capture. +TEST(CodeCacheTest, MemoryUsageGrowsWithNameLength) { + CodeCache shortc("lib"); + CodeCache longc("lib"); + long long short_base = shortc.memoryUsage(); + long long long_base = longc.memoryUsage(); + + shortc.add((const void *)0x1000, 8, "f"); + longc.add((const void *)0x1000, 8, + "an_extremely_long_symbol_name_padded_out_further_and_further_1234"); + + EXPECT_GT(longc.memoryUsage() - long_base, shortc.memoryUsage() - short_base); +} + +// The blob array is counted at the full CodeBlob size, not a pointer's worth +// (the previous formula used sizeof(CodeBlob*), undercounting ~3x). An empty +// cache already accounts for capacity * sizeof(CodeBlob) plus its own name. +TEST(CodeCacheTest, MemoryUsageCountsFullBlobArray) { + CodeCache cc("lib"); + long long usage = cc.memoryUsage(); + EXPECT_GT(usage, (long long)(INITIAL_CODE_CACHE_CAPACITY * sizeof(CodeBlob))); + // Sanity: a pointer-sized count would be far smaller than the real array. + EXPECT_GT((long long)(INITIAL_CODE_CACHE_CAPACITY * sizeof(CodeBlob)), + (long long)(INITIAL_CODE_CACHE_CAPACITY * sizeof(CodeBlob *))); +} From 8d1c750d9fb7130448dc1c9739e821a9f1cc73b2 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 21 Jul 2026 14:07:51 +0200 Subject: [PATCH 2/3] docs(codecache): describe current memoryUsage(), drop old-formula references Comments explained what the previous formula did wrong rather than what the code now counts. Reword to describe the current behavior only. Co-Authored-By: Claude Opus 4.8 (1M context) --- ddprof-lib/src/main/cpp/codeCache.cpp | 11 +++++------ ddprof-lib/src/main/cpp/codeCache.h | 15 ++++++++------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index d460e5f6b0..5f36c0f64a 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -145,19 +145,18 @@ CodeCache::~CodeCache() { } long long CodeCache::memoryUsage() { - // Blob array. The previous formula used sizeof(CodeBlob*), undercounting the - // array roughly threefold (CodeBlob holds three pointers). + // The blob array: _capacity entries of CodeBlob. long long total = (long long)_capacity * sizeof(CodeBlob); - // This cache's own name, plus each symbol's variable-length name string. The - // previous formula approximated these as a fixed sizeof(NativeFunc), ignoring - // the name length that dominates the allocation. + // This cache's own name, plus each symbol's name string. Each is a + // variable-length allocation whose size depends on the name length + // (see NativeFunc::allocSize). total += (long long)NativeFunc::allocSize(_name); for (int i = 0; i < _count; i++) { total += (long long)NativeFunc::allocSize(_blobs[i]._name); } - // DWARF unwind table and build-id string were not counted at all before. + // The DWARF unwind table and the build-id string, when present. total += (long long)_dwarf_table_length * sizeof(FrameDesc); if (_build_id != nullptr) { total += (long long)strlen(_build_id) + 1; diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index d6ef7955a2..2b49cb41ee 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -260,10 +260,10 @@ class CodeCache { void setDwarfTable(FrameDesc *table, int length, const FrameDesc &default_frame = FrameDesc::default_frame); FrameDesc findFrameDesc(const void *pc); - // Accurate live size of everything this CodeCache owns on the heap: the blob - // array, the per-symbol name strings (variable length), the DWARF unwind - // table, the build-id string, and this cache's own name. Recomputed on - // demand (called only at dump time), so it always reflects current contents. + // Live size of everything this CodeCache owns on the heap: the blob array, + // the per-symbol name strings (variable length), the DWARF unwind table, the + // build-id string, and this cache's own name. Recomputed on demand (called + // only at dump time), so it reflects the current contents. long long memoryUsage(); int count() { return _count; } @@ -326,9 +326,10 @@ class CodeCacheArray { } // Sum the live memory of all registered libraries. Recomputed on demand - // (called only at dump time) so it tracks each library's later growth, - // unlike a value cached at add() time. The array is append-only, so - // iterating the published prefix is safe alongside concurrent add()s. + // (called only at dump time) so it reflects each library's current size, + // including symbols added after the library was registered. The array is + // append-only, so iterating the published prefix is safe alongside + // concurrent add()s. size_t memoryUsage() const { size_t total = 0; int n = count(); From cf512dde50e1b2f2f05058076b7451478e5203a9 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 21 Jul 2026 15:56:45 +0200 Subject: [PATCH 3/3] fix(codecache): don't read build-id in memoryUsage() (data race); review nits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review feedback: - Drop the build-id from memoryUsage(). The background library refresher (Libraries::updateBuildIds) calls setBuildId() — which frees and replaces _build_id — on already-published caches under _build_id_lock, which dump does not hold. The strlen(_build_id) added here could race that free/replace (use-after-free / torn read). Build-id is negligible (~tens of bytes per library) next to the symbol tables, so excluding it keeps the read lock-free at no meaningful accuracy cost. The blob-name loop and dwarf-length read are safe: blobs are fixed once a library is published (same read the symbolication fast path does) and the dwarf read touches only the length scalar. - Mark CodeCache::memoryUsage() const (it does not mutate state). - Test: use reinterpret_cast for the integer-to-pointer args and drop the unused include. Co-Authored-By: Claude Opus 4.8 (1M context) --- ddprof-lib/src/main/cpp/codeCache.cpp | 18 ++++++++++++------ ddprof-lib/src/main/cpp/codeCache.h | 13 ++++++++----- ddprof-lib/src/test/cpp/codeCache_ut.cpp | 17 ++++++++--------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 5f36c0f64a..a18d8c2e68 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -144,23 +144,29 @@ CodeCache::~CodeCache() { free(_build_id); // Free build-id memory } -long long CodeCache::memoryUsage() { +long long CodeCache::memoryUsage() const { // The blob array: _capacity entries of CodeBlob. long long total = (long long)_capacity * sizeof(CodeBlob); // This cache's own name, plus each symbol's name string. Each is a // variable-length allocation whose size depends on the name length - // (see NativeFunc::allocSize). + // (see NativeFunc::allocSize). Both the name pointers and the array are fixed + // once the library is published (the same read the symbolication fast path + // does), so this is safe to read at dump time without extra synchronization. total += (long long)NativeFunc::allocSize(_name); for (int i = 0; i < _count; i++) { total += (long long)NativeFunc::allocSize(_blobs[i]._name); } - // The DWARF unwind table and the build-id string, when present. + // The DWARF unwind table, when present (length only — no pointer deref). total += (long long)_dwarf_table_length * sizeof(FrameDesc); - if (_build_id != nullptr) { - total += (long long)strlen(_build_id) + 1; - } + + // The build-id string is intentionally NOT counted here: the background + // library refresher (Libraries::updateBuildIds) frees and replaces _build_id + // on already-published caches under _build_id_lock, which dump does not hold, + // so dereferencing it here would race. It is negligible (~tens of bytes per + // library) next to the symbol tables, so excluding it costs no meaningful + // accuracy while keeping this read lock-free. return total; } diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index 2b49cb41ee..bfa65c6601 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -260,11 +260,14 @@ class CodeCache { void setDwarfTable(FrameDesc *table, int length, const FrameDesc &default_frame = FrameDesc::default_frame); FrameDesc findFrameDesc(const void *pc); - // Live size of everything this CodeCache owns on the heap: the blob array, - // the per-symbol name strings (variable length), the DWARF unwind table, the - // build-id string, and this cache's own name. Recomputed on demand (called - // only at dump time), so it reflects the current contents. - long long memoryUsage(); + // Live size of what this CodeCache owns on the heap: the blob array, the + // per-symbol name strings (variable length), this cache's own name, and the + // DWARF unwind table. Recomputed on demand (called only at dump time), so it + // reflects the current contents. The build-id string is deliberately excluded + // — it is mutated by the background refresher and negligible in size (see the + // definition). Const and lock-free: reads only fields that are stable once the + // library is published. + long long memoryUsage() const; int count() { return _count; } CodeBlob* blob(int idx) { diff --git a/ddprof-lib/src/test/cpp/codeCache_ut.cpp b/ddprof-lib/src/test/cpp/codeCache_ut.cpp index 039de04b0f..a679645573 100644 --- a/ddprof-lib/src/test/cpp/codeCache_ut.cpp +++ b/ddprof-lib/src/test/cpp/codeCache_ut.cpp @@ -5,7 +5,6 @@ #include #include "codeCache.h" -#include // memoryUsage() must reflect the actual per-symbol name length, not a fixed // per-symbol constant: adding one symbol grows the reported usage by exactly @@ -16,29 +15,29 @@ TEST(CodeCacheTest, MemoryUsageCountsSymbolNameLength) { const char *name = "a_long_symbol_name_well_beyond_sizeof_NativeFunc_padding_xxxxxxxxxxxx"; - cc.add((const void *)0x1000, 16, name); + cc.add(reinterpret_cast(0x1000), 16, name); EXPECT_EQ(NativeFunc::allocSize(name), (size_t)(cc.memoryUsage() - base)); } -// A longer name costs more than a shorter one — i.e. length actually matters, -// which the old fixed sizeof(NativeFunc) formula could not capture. +// A longer name costs more than a shorter one — the reported size scales with +// the actual symbol-name length. TEST(CodeCacheTest, MemoryUsageGrowsWithNameLength) { CodeCache shortc("lib"); CodeCache longc("lib"); long long short_base = shortc.memoryUsage(); long long long_base = longc.memoryUsage(); - shortc.add((const void *)0x1000, 8, "f"); - longc.add((const void *)0x1000, 8, + shortc.add(reinterpret_cast(0x1000), 8, "f"); + longc.add(reinterpret_cast(0x1000), 8, "an_extremely_long_symbol_name_padded_out_further_and_further_1234"); EXPECT_GT(longc.memoryUsage() - long_base, shortc.memoryUsage() - short_base); } -// The blob array is counted at the full CodeBlob size, not a pointer's worth -// (the previous formula used sizeof(CodeBlob*), undercounting ~3x). An empty -// cache already accounts for capacity * sizeof(CodeBlob) plus its own name. +// The blob array is counted at the full CodeBlob size (a CodeBlob is several +// pointers wide): an empty cache already accounts for +// capacity * sizeof(CodeBlob) plus its own name. TEST(CodeCacheTest, MemoryUsageCountsFullBlobArray) { CodeCache cc("lib"); long long usage = cc.memoryUsage();