From 97754b1378d567cd2f14fef8d18e4f66f8246449 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Mon, 24 Aug 2026 15:00:03 +0000 Subject: [PATCH 1/3] fix(nativemem): refresh NM_NATIVE_SYMBOLS where the tables change NM_NATIVE_SYMBOLS was written only by Profiler::updateNativeLibMemStats(), which runs solely from Profiler::stop() and Profiler::dump(). Neither fires during a recording, so the gauge held its initial 0 for the entire life of a profiled process while the native-symbol tables really held ~13 MB. The profiler under-reported its own native memory by that much at every steady-state sample point, and any reconciliation of RSS against the counters was subtracting zero for memory that was really there. Libraries::updateSymbols() is the only function that grows _native_libs, and a published CodeCache is immutable -- add(), expand() and setDwarfTable() each assert they run before publication. The gauge therefore does not need polling: recomputing it where the tables change is sufficient for it to be accurate at any later instant. That also covers every path that can change them: agent load, profiler start, the background refresher's refresh(), and the kernel-symbol parse. CodeCache::memoryUsage() itself needed no change; it was correct and simply never evaluated. Measured with the allocation ledger at N=150,000, steady state, both reps byte-identical. Counted malloc-backed bytes rise from 8.30 to 21.32 MiB against 22.21 MiB actually allocated, so uncounted falls from 13.91 MiB (63%) to 0.88 MiB (4%); with the library loaded but profiling not started it falls from 13.30 MiB to 0.27 MiB. This also absorbs most of the 3.64 MiB that previously resolved only to the statically-linked private operator new -- those were `new CodeBlob[_capacity]` arrays, covered by memoryUsage()'s capacity term. Counter-only change: it allocates nothing, class-load time is unchanged at 12-14 s, and RSS is unchanged within noise. Co-Authored-By: Claude Opus 5 (1M context) --- ddprof-lib/src/main/cpp/libraries.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ddprof-lib/src/main/cpp/libraries.cpp b/ddprof-lib/src/main/cpp/libraries.cpp index 66382e9d1b..c762131609 100644 --- a/ddprof-lib/src/main/cpp/libraries.cpp +++ b/ddprof-lib/src/main/cpp/libraries.cpp @@ -10,6 +10,7 @@ #include "libraryPatcher.h" #include "log.h" #include "mallocTracer.h" +#include "nativeMem.h" #include "os.h" #include "profiler.h" #include "symbols.h" @@ -60,6 +61,19 @@ void Libraries::mangle(const char *name, char *buf, size_t size) { void Libraries::updateSymbols(bool kernel_symbols) { Symbols::parseLibraries(&_native_libs, kernel_symbols); LibraryPatcher::patch_libraries(); + // Refresh the native-symbol memory gauge here, where the tables it measures + // actually change. This is the only function that grows _native_libs, and a + // published CodeCache is immutable -- add(), expand() and setDwarfTable() all + // assert they run before publication -- so recomputing at this point is + // sufficient for the gauge to be accurate at any later instant. + // + // Previously NM_NATIVE_SYMBOLS was written only by + // Profiler::updateNativeLibMemStats(), which runs solely from Profiler::stop() + // and Profiler::dump(). Neither fires during a recording, so the gauge read 0 + // for the entire life of a profiled process while these tables really held + // ~10 MB, making the profiler under-report its own native memory by that much + // at every steady-state sample point. + NativeMem::setLive(NM_NATIVE_SYMBOLS, (long long)_native_libs.memoryUsage()); } void Libraries::refresh() { From de0c23a596d3c01c8f3d670ae6562b5e1428ac80 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Tue, 25 Aug 2026 13:56:03 +0000 Subject: [PATCH 2/3] fix(nativemem): make NM_NATIVE_SYMBOLS event-driven instead of a periodic rescan The previous fix (making the gauge accurate at all during a recording, not just at stop()/dump()) worked by recomputing CodeCacheArray::memoryUsage() from scratch -- an O(total symbols across every loaded library) rescan -- and overwriting the gauge with NativeMem::setLive() on every dlopen refresh. Two real problems with that, caught in PR #753 review: the rescan cost scales with cumulative dlopen activity, not just new content: and Profiler::dump() (profiler.cpp) can call Libraries::refresh() concurrently with the background refresher thread's own refresh(), since neither takes a common lock -- so whichever setLive() finishes last wins, even if it started first with a smaller snapshot. Replaces both with incremental accounting matching every other NM_* category: CodeCache now tracks a running _memory_usage total, updated by the same pre-publication mutators (constructor, add(), expand(), setDwarfTable()) that already assert they never run after publication. NativeMem::record() -- a relaxed atomic add -- fires exactly once per library, at the moment CodeCacheArray::add() publishes it. A cache discarded before publication (Symbols::parseLibraries deletes one directly on parse failure/duplicate detection) was simply never recorded, so no matching decrement is needed. memoryUsage() itself drops from an O(symbols) loop to an O(1) field read. Verified: NM_NATIVE_SYMBOLS reads the same 13.03 MiB as the rescan-based version on an identical workload; counter-audit coverage unchanged (95.5%, +0.89 MiB unaccounted, same as before). codeCache_ut (release+debug), libraries_ut, nativeMem_ut, and stackWalker_ut all pass. Co-Authored-By: Claude Sonnet 5 --- ddprof-lib/src/main/cpp/codeCache.cpp | 54 +++++++++++++-------------- ddprof-lib/src/main/cpp/codeCache.h | 30 ++++++++++++--- ddprof-lib/src/main/cpp/libraries.cpp | 27 +++++++------- ddprof-lib/src/main/cpp/profiler.cpp | 15 ++++++-- 4 files changed, 76 insertions(+), 50 deletions(-) diff --git a/ddprof-lib/src/main/cpp/codeCache.cpp b/ddprof-lib/src/main/cpp/codeCache.cpp index 9062b555b1..f8933682ea 100644 --- a/ddprof-lib/src/main/cpp/codeCache.cpp +++ b/ddprof-lib/src/main/cpp/codeCache.cpp @@ -71,6 +71,9 @@ CodeCache::CodeCache(const char *name, short lib_index, _count = 0; _blobs = new CodeBlob[_capacity]; + _memory_usage = (long long)_capacity * sizeof(CodeBlob) + + (long long)NativeFunc::allocSize(_name); + _published.store(false, std::memory_order_relaxed); } @@ -125,6 +128,11 @@ void CodeCache::copyFrom(const CodeCache& other) { } } + // Identical content to `other`, so its running total transfers as-is -- + // no need to recompute (and `other` may itself already be published, whose + // _blobs this copy must not depend on after construction). + _memory_usage = other._memory_usage; + // A copy is a fresh, not-yet-registered cache. _published.store(false, std::memory_order_relaxed); } @@ -159,35 +167,19 @@ CodeCache::~CodeCache() { } 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). The lock-free read here is safe only for caches - // registered in a CodeCacheArray (Libraries::native_libs): their _blobs array - // and name pointers are fixed once published (add()/expand()/setDwarfTable() - // run only pre-publish — asserted via _published), which is the same read the - // symbolication fast path relies on. It must NOT be called on a continuously - // mutated, unpublished cache such as JitCodeCache::_runtime_stubs without - // holding JitCodeCache::_stubs_lock (shared), since a concurrent add()/expand() - // would free _blobs underneath the reader. - 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, when present (length only — no pointer deref). - total += (long long)_dwarf_table_length * sizeof(FrameDesc); - - // 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; + // O(1): _memory_usage is updated incrementally by the constructor, add(), + // expand(), and setDwarfTable() -- the same pre-publication mutators that + // used to be re-summed here on every call. Safe to read lock-free once + // published (Libraries::native_libs) because those mutators never run + // afterwards (asserted via _published), same invariant the old per-call + // rescan relied on. + // + // Deliberately excludes the build-id string: 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 folding it into this running total would race. It is negligible + // (~tens of bytes per library) next to the symbol tables. + return _memory_usage; } void CodeCache::expand() { @@ -200,6 +192,8 @@ void CodeCache::expand() { memcpy(new_blobs, old_blobs, _count * sizeof(CodeBlob)); + // Delta is exactly the current (pre-doubling) capacity's worth of blobs. + _memory_usage += (long long)_capacity * sizeof(CodeBlob); _capacity *= 2; _blobs = new_blobs; delete[] old_blobs; @@ -214,6 +208,7 @@ void CodeCache::add(const void *start, int length, const char *name, assert(!_published.load(std::memory_order_acquire) && "add() on a published CodeCache races memoryUsage()"); char *name_copy = NativeFunc::create(name, _lib_index); + _memory_usage += (long long)NativeFunc::allocSize(name); // Replace non-printable characters for (char *s = name_copy; *s != 0; s++) { if (*s < ' ') @@ -478,6 +473,7 @@ void CodeCache::setDwarfTable(FrameDesc *table, int length, const FrameDesc &def // buffer to exactly `length` entries before returning it from table(), so // memoryUsage()'s length-based formula matches the real allocation with no // trim step needed here. + _memory_usage += (long long)length * sizeof(FrameDesc); _dwarf_table = table; _dwarf_table_length = length; _default_frame = &default_frame; diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h index d8ac7d661e..db7de21f67 100644 --- a/ddprof-lib/src/main/cpp/codeCache.h +++ b/ddprof-lib/src/main/cpp/codeCache.h @@ -10,6 +10,7 @@ #include "common.h" #include "counters.h" #include "dwarf.h" +#include "nativeMem.h" #include "utils.h" #include @@ -164,6 +165,14 @@ class CodeCache { int _count; CodeBlob *_blobs; + // Running total of memoryUsage()'s formula, updated incrementally by the + // same pre-publication mutators (constructor, add(), expand(), + // setDwarfTable()) instead of recomputed by iterating _blobs. Safe because + // every contributing field is fixed before publication (see _published + // below) -- there is no point after construction-and-population where this + // total could be stale relative to a fresh recompute. + long long _memory_usage; + // Set once the cache is registered into a CodeCacheArray (see markPublished()). // After that, memoryUsage() may be read lock-free from another thread (dump), // so the mutators that touch the fields it reads (add()/expand()/ @@ -328,6 +337,17 @@ 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); + // Record this library's contribution to NM_NATIVE_SYMBOLS exactly once, + // right here at publication -- not via a periodic recompute-and-overwrite + // elsewhere. A cache that fails to publish (the overflow branch above) is + // simply never recorded, so there is no corresponding decrement to get + // right on the parse-failure/duplicate discard path in + // Symbols::parseLibraries (which deletes an unpublished cache directly). + // Because record() is a relaxed atomic add, concurrent publishers from + // different threads (or a publish racing Profiler::dump()'s read of the + // aggregate) can never clobber each other's contribution -- unlike the + // read-modify-write a periodic setLive(sum-of-everything) would need. + NativeMem::record(NM_NATIVE_SYMBOLS, lib->memoryUsage()); // Mark published before the RELEASE store makes the pointer visible, so any // later add()/expand()/setDwarfTable() on this cache trips the assert (its // _blobs would then be read lock-free by memoryUsage() at dump time). @@ -351,11 +371,11 @@ 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 reflects each library's fully-populated - // state at that point — the accurate per-library formula rather than a value - // cached at add() time. (Libraries are populated before being registered and - // not grown afterwards.) The array is append-only, so iterating the published + // Sum the live memory of all registered libraries. Each lib->memoryUsage() + // is now an O(1) read of a running total (see CodeCache::_memory_usage), + // not a rescan of its symbol table, so this whole sum is O(libraries) -- + // still only called at dump time, but no longer the reason to avoid calling + // it more often. The array is append-only, so iterating the published // prefix is safe alongside concurrent add()s. size_t memoryUsage() const { size_t total = 0; diff --git a/ddprof-lib/src/main/cpp/libraries.cpp b/ddprof-lib/src/main/cpp/libraries.cpp index c762131609..b9bd21e9aa 100644 --- a/ddprof-lib/src/main/cpp/libraries.cpp +++ b/ddprof-lib/src/main/cpp/libraries.cpp @@ -10,7 +10,6 @@ #include "libraryPatcher.h" #include "log.h" #include "mallocTracer.h" -#include "nativeMem.h" #include "os.h" #include "profiler.h" #include "symbols.h" @@ -61,19 +60,21 @@ void Libraries::mangle(const char *name, char *buf, size_t size) { void Libraries::updateSymbols(bool kernel_symbols) { Symbols::parseLibraries(&_native_libs, kernel_symbols); LibraryPatcher::patch_libraries(); - // Refresh the native-symbol memory gauge here, where the tables it measures - // actually change. This is the only function that grows _native_libs, and a - // published CodeCache is immutable -- add(), expand() and setDwarfTable() all - // assert they run before publication -- so recomputing at this point is - // sufficient for the gauge to be accurate at any later instant. + // NM_NATIVE_SYMBOLS accounting happens per-library, at the moment each + // CodeCache is published (CodeCacheArray::add(), codeCache.h) -- not here. // - // Previously NM_NATIVE_SYMBOLS was written only by - // Profiler::updateNativeLibMemStats(), which runs solely from Profiler::stop() - // and Profiler::dump(). Neither fires during a recording, so the gauge read 0 - // for the entire life of a profiled process while these tables really held - // ~10 MB, making the profiler under-report its own native memory by that much - // at every steady-state sample point. - NativeMem::setLive(NM_NATIVE_SYMBOLS, (long long)_native_libs.memoryUsage()); + // An earlier version of this fix recomputed and overwrote the whole gauge + // from this function on every call (each dlopen refresh, not just + // stop()/dump() as before it existed at all -- see git history for why the + // gauge previously read 0 for a recording's entire lifetime). Two review + // findings on that approach: it turned every dlopen into an O(total symbols + // across every loaded library) rescan instead of O(new symbols only), and a + // dump()-triggered refresh could race the background refresher thread's own + // refresh() and overwrite a newer total with a stale smaller one (neither + // path takes a common lock). The publish-time NativeMem::record() approach + // has neither problem: each publish is an O(1) atomic add of that one + // library's already-computed total, so there is nothing to rescan and + // nothing to race. } void Libraries::refresh() { diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index 7f9f208bc0..4b3bdb0a46 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -1889,8 +1889,18 @@ Error Profiler::check(Arguments &args) { void Profiler::updateNativeLibMemStats() { // CodeCache here is the profiler's native-symbol tables (not the JVM code - // cache). memoryUsage() is a recomputed gauge; read it once and publish the - // counters plus the NativeMem gauge (NATIVE_SYMBOLS) as absolutes. + // cache). memoryUsage() is now an O(1) read of each library's running + // total (see CodeCache::_memory_usage), not a rescan, so reading it here + // for the Counters:: mirrors below is cheap even though this itself is + // only called from stop()/dump(). + // + // Deliberately does NOT also write NM_NATIVE_SYMBOLS here: that gauge is + // maintained incrementally at publish time (CodeCacheArray::add(), an + // atomic add per library). Overwriting it with a fresh recompute from this + // function -- which can run concurrently with the background refresher + // thread publishing a new library, since neither takes a common lock -- + // would reintroduce exactly the stale-overwrite race the publish-time + // accounting was built to avoid. const CodeCacheArray& native_libs = _libs->native_libs(); long long usage = (long long)native_libs.memoryUsage(); Counters::set(CODECACHE_NATIVE_COUNT, native_libs.count()); @@ -1900,7 +1910,6 @@ void Profiler::updateNativeLibMemStats() { // behind the VM abstraction (0 on J9/Zing). Counters::set(CODECACHE_RUNTIME_STUBS_SIZE_BYTES, JVMSupport::runtimeStubsMemoryUsage()); - NativeMem::setLive(NM_NATIVE_SYMBOLS, usage); } Error Profiler::dump(const char *path, const int length) { From d63b0e7e1ce474854b310d93693f0e505b82c8c7 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Wed, 26 Aug 2026 13:18:01 +0000 Subject: [PATCH 3/3] docs: reword comment to state the design rationale directly, not refer to PR review history The comment referenced "an earlier version of this fix", "two review findings", and "see git history" -- context specific to this PR's review thread and iteration history, not visible to someone reading the code later (after merge, or without pulling up this PR). Restated the two underlying technical problems (rescan cost, race window) as facts about the design space instead. Co-Authored-By: Claude Sonnet 5 --- ddprof-lib/src/main/cpp/libraries.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/ddprof-lib/src/main/cpp/libraries.cpp b/ddprof-lib/src/main/cpp/libraries.cpp index b9bd21e9aa..a36edf2b5f 100644 --- a/ddprof-lib/src/main/cpp/libraries.cpp +++ b/ddprof-lib/src/main/cpp/libraries.cpp @@ -63,18 +63,17 @@ void Libraries::updateSymbols(bool kernel_symbols) { // NM_NATIVE_SYMBOLS accounting happens per-library, at the moment each // CodeCache is published (CodeCacheArray::add(), codeCache.h) -- not here. // - // An earlier version of this fix recomputed and overwrote the whole gauge - // from this function on every call (each dlopen refresh, not just - // stop()/dump() as before it existed at all -- see git history for why the - // gauge previously read 0 for a recording's entire lifetime). Two review - // findings on that approach: it turned every dlopen into an O(total symbols - // across every loaded library) rescan instead of O(new symbols only), and a - // dump()-triggered refresh could race the background refresher thread's own - // refresh() and overwrite a newer total with a stale smaller one (neither - // path takes a common lock). The publish-time NativeMem::record() approach - // has neither problem: each publish is an O(1) atomic add of that one - // library's already-computed total, so there is nothing to rescan and - // nothing to race. + // A periodic recompute-and-overwrite from this function would have two + // problems. First, it would turn every dlopen refresh into an O(total + // symbols across every loaded library) rescan instead of O(new symbols + // only). Second, a dump()-triggered refresh (Profiler::dump()) can run + // concurrently with the background refresher thread's own refresh() -- + // neither takes a common lock -- so whichever recompute-and-overwrite + // finishes last would win even if it started first with a smaller, stale + // total. The publish-time NativeMem::record() approach has neither + // problem: each publish is an O(1) atomic add of that one library's + // already-computed total, so there is nothing to rescan and nothing to + // race. } void Libraries::refresh() {