fix(codecache): make memoryUsage() accurate and live#677
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves native-symbol memory accounting by making CodeCache::memoryUsage() (and the aggregated CodeCacheArray::memoryUsage()) recompute live, accurate heap usage for native symbol tables, which feeds the CODECACHE_NATIVE_SIZE_BYTES counter.
Changes:
- Replaces the old approximation in
CodeCache::memoryUsage()with a live recompute that accounts for the full blob array, variable-length symbol-name allocations, DWARF unwind table, build-id string, and the cache’s own name. - Removes
CodeCacheArray’s cached_used_memoryand sums per-librarymemoryUsage()at call time to reflect post-registration growth. - Adds
codeCache_utcoverage validating that memory usage scales with symbol-name length and counts the fullCodeBlobarray size.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ddprof-lib/src/test/cpp/codeCache_ut.cpp | Adds focused unit tests for memoryUsage() accuracy and growth behavior. |
| ddprof-lib/src/main/cpp/codeCache.h | Introduces NativeFunc::allocSize(), updates CodeCache::memoryUsage() API, and makes CodeCacheArray::memoryUsage() sum live values. |
| ddprof-lib/src/main/cpp/codeCache.cpp | Implements the new accurate, live CodeCache::memoryUsage() calculation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const char *name = | ||
| "a_long_symbol_name_well_beyond_sizeof_NativeFunc_padding_xxxxxxxxxxxx"; | ||
| cc.add((const void *)0x1000, 16, name); |
| shortc.add((const void *)0x1000, 8, "f"); | ||
| longc.add((const void *)0x1000, 8, | ||
| "an_extremely_long_symbol_name_padded_out_further_and_further_1234"); |
CI Test ResultsRun: #29836797137 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 31 | Failed: 0 | Cancelled: 1 Updated: 2026-07-21 17:00:46 UTC |
Benchmark Results (commit cf42bb1)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125921385 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10280 ms (21 iters) | ✅ 10270 ms (21 iters) | ≈ -0.1% (±11.8%) | — / — |
| akka-uct | 25 | ✅ 8817 ms (24 iters) | ✅ 8918 ms (24 iters) | ≈ +1.1% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 6040 ms (33 iters) | ✅ 5974 ms (33 iters) | ≈ -1.1% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5505 ms (36 iters) | ✅ 5373 ms (36 iters) | ≈ -2.4% (±24.2%) | |
| fj-kmeans | 21 | ✅ 2643 ms (71 iters) | ✅ 2697 ms (70 iters) | ≈ +2% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2808 ms (66 iters) | ✅ 2823 ms (66 iters) | ≈ +0.5% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2042 ms (90 iters) | ✅ 2078 ms (89 iters) | ≈ +1.8% (±2.5%) | — / — |
| future-genetic | 25 | ✅ 2088 ms (89 iters) | ✅ 2006 ms (93 iters) | 🟢 -3.9% | — / — |
| naive-bayes | 21 | ✅ 1288 ms (133 iters) | ✅ 1253 ms (137 iters) | ≈ -2.7% (±31.8%) | — / — |
| naive-bayes | 25 | ✅ 1058 ms (162 iters) | ✅ 991 ms (172 iters) | ≈ -6.3% (±30.6%) | — / — |
| reactors | 21 | ✅ 16460 ms (15 iters) | ✅ 16646 ms (15 iters) | ≈ +1.1% (±7.5%) | — / — |
| reactors | 25 | ✅ 18515 ms (15 iters) | ✅ 18521 ms (15 iters) | ≈ +0% (±4.2%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 1950 / 1959 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2209 / 2042 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 8695 / 8520 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8170 / 8562 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1278 / 1297 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1258 / 1270 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2890 / 2997 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2919 / 2906 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 3530 / 3526 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 7 | 3483 / 3505 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1692 / 1796 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1904 / 1851 | ✅ / ✅ | ✅ / ✅ |
…erences 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) <noreply@anthropic.com>
![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: 8d1c750d9f
ℹ️ 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".
| if (_build_id != nullptr) { | ||
| total += (long long)strlen(_build_id) + 1; |
There was a problem hiding this comment.
When remote symbolication is enabled, the background refresher can call Libraries::updateBuildIds() on already-published CodeCache objects while Profiler::dump() is computing these counters. setBuildId() assigns _build_id before copying the string into that allocation, but this new strlen(_build_id) runs without the _build_id_lock, so a concurrent dump can scan an uninitialized buffer and potentially read past the allocation or crash. Please publish build IDs only after initialization with synchronization/atomic ordering, or avoid dereferencing this mutable field without the same synchronization.
Useful? React with 👍 / 👎.
| // 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; | ||
| } |
| // 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(); |
Benchmark Results (commit 8d1c750)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125932438 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10391 ms (21 iters) | ✅ 10300 ms (21 iters) | ≈ -0.9% (±11.5%) | — / — |
| akka-uct | 25 | ✅ 8954 ms (24 iters) | ✅ 8757 ms (24 iters) | ≈ -2.2% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5979 ms (33 iters) | ✅ 5961 ms (33 iters) | ≈ -0.3% (±25.1%) | |
| finagle-chirper | 25 | ✅ 5511 ms (36 iters) | ✅ 5514 ms (36 iters) | ≈ +0.1% (±25%) | |
| fj-kmeans | 21 | ✅ 2701 ms (69 iters) | ✅ 2820 ms (66 iters) | 🔴 +4.4% | — / — |
| fj-kmeans | 25 | ✅ 2837 ms (66 iters) | ✅ 2830 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2099 ms (88 iters) | ✅ 2071 ms (90 iters) | ≈ -1.3% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2079 ms (89 iters) | ✅ 2070 ms (90 iters) | ≈ -0.4% (±2.7%) | — / — |
| naive-bayes | 21 | ✅ 1288 ms (133 iters) | ✅ 1305 ms (131 iters) | ≈ +1.3% (±32.9%) | — / — |
| naive-bayes | 25 | ✅ 1027 ms (166 iters) | ✅ 1027 ms (166 iters) | ≈ 0% (±31.8%) | — / — |
| reactors | 21 | ✅ 16180 ms (15 iters) | ✅ 17360 ms (15 iters) | 🔴 +7.3% | — / — |
| reactors | 25 | ✅ 18388 ms (15 iters) | ✅ 18296 ms (15 iters) | ≈ -0.5% (±4.7%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2029 / 1937 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 2274 / 2377 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 8644 / 8323 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 8369 / 8619 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 1282 / 1263 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2890 / 3007 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2938 / 2923 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 6 / 3 | 3490 / 3481 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 4 | 3496 / 3474 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1697 / 1771 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1900 / 1864 | ✅ / ✅ | ✅ / ✅ |
…iew nits 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 <cstring> include. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the reviews — addressed in cf512dd: Build-id data race (codex P2 / Copilot) — Good catch.
Use Re: the benchmark outliers ( |
| // 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. |
Benchmark Results (commit cf512dd)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125969722 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
What
Makes
CodeCache::memoryUsage()— which drives theCODECACHE_NATIVE_SIZE_BYTEScounter — report the profiler's actual native-symbol memory, instead of a stale, heavily-undercounted approximation.(
CodeCachehere is the profiler's per-native-library symbol table used to symbolicate native frames — async-profiler's naming, not the JVM's JIT code cache.)Why
The old formula
_capacity * sizeof(CodeBlob*) + _count * sizeof(NativeFunc)was wrong in several ways:sizeof(CodeBlob*)(a pointer, 8 B) where the array actually holdsCodeBlob(three pointers, 24 B).sizeof(NativeFunc)(4 B) per symbol, ignoring the name-string length that dominates each allocation (aligned_alloc(sizeof(NativeFunc) + 1 + strlen(name)))._dwarf_table, can be large) and the build-id string.CodeCacheArraycached the sum atadd()time via_used_memory, so it never reflected a library's later symbol growth.How
CodeCache::memoryUsage()recomputes accurately: full blob array (sizeof(CodeBlob)), each symbol's real name allocation (newNativeFunc::allocSize()), this cache's own name, the DWARF table, and the build-id.CodeCacheArray::memoryUsage()sums the live per-library values instead of a cached total. The array is append-only, so iterating the published prefix is safe alongside concurrentadd()s. The now-dead_used_memoryfield and itsadd()-time accumulation are removed.memoryUsage()is only called at dump time, so theO(symbols)recompute is off any hot path.Testing
codeCache_ut(3 tests): usage tracks per-symbol name length exactly; a longer name costs more than a shorter one; the blob array is counted at fullCodeBlobsize, not a pointer's.:ddprof-lib:gtestDebugsuite green — 352 tests, 0 failures.Note
Independent of the native-memory accounting work in #669; that PR's
NM_NATIVE_SYMBOLSgauge mirrors thismemoryUsage(), so it benefits automatically once both land, but neither depends on the other to build.🤖 Generated with Claude Code