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 66382e9d1b..a36edf2b5f 100644 --- a/ddprof-lib/src/main/cpp/libraries.cpp +++ b/ddprof-lib/src/main/cpp/libraries.cpp @@ -60,6 +60,20 @@ 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(); + // NM_NATIVE_SYMBOLS accounting happens per-library, at the moment each + // CodeCache is published (CodeCacheArray::add(), codeCache.h) -- not here. + // + // 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() { 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) {