fix(codecache): make memoryUsage() accurate and live#677
Conversation
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.
CI Test ResultsRun: #29932410242 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-22 16:00:22 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 | ✅ / ✅ | ✅ / ✅ |
![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".
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 | ✅ / ✅ | ✅ / ✅ |
|
Thanks for the reviews — addressed in cf512dd: Build-id data race (codex P2 / Copilot) — Good catch.
Use Re: the benchmark outliers ( |
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):
|
|
Re: the PR-description / code mismatch on build-id (Copilot @ codeCache.cpp:169) — went with option (b): updated the PR description so it no longer lists build-id as counted, and added an explicit note that it's intentionally excluded to avoid the Also noting the benchmark run for cf512dd came back "within expected boundaries — all within run-to-run noise," which lines up with the earlier assessment that the outliers on prior commits were noise. |
This comment has been minimized.
This comment has been minimized.
zhengyu123
left a comment
zhengyu123
left a comment
There was a problem hiding this comment.
Looks good overall.
Unrelated to this PR:
Doubled cost at the call site — profiler.cpp:1739-1741
Counters::set(CODECACHE_NATIVE_SIZE_BYTES, native_libs.memoryUsage());
Counters::set(CODECACHE_RUNTIME_STUBS_SIZE_BYTES,
native_libs.memoryUsage());
|
Thanks for the review! On the unrelated note ( Separately, while there: |
|
Thanks for the thorough Sphinx pass — all seven addressed ( MEDIUM
LOW
Full |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/codeCache.cpp:179
- The build-id exclusion rationale is inaccurate:
Libraries::updateBuildIds()only callssetBuildId()once per library (see_build_id_processed+hasBuildId()), so it does not "free and replace" build-ids repeatedly. If the concern is unsynchronized publication to dump-time readers, clarify that instead (otherwise this comment suggests a different concurrency hazard than what the code actually does).
// 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.
|
Good catch — fixed in the latest commit. My comments named |
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>
…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>
…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>
Address the Sphinx review on memoryUsage()'s lock-free read: - Make memoryUsage()'s lock-free safety an enforced precondition. Add a _published flag, set by CodeCacheArray::add() before the pointer is published, and assert !_published in add()/expand()/setDwarfTable() — the mutators that touch the _blobs array and _dwarf_table_length that memoryUsage() reads from the dump thread. Standalone caches that are never registered (e.g. Libraries::_runtime_stubs, which is continuously mutated under its own lock) stay unpublished and may be mutated freely. - Scope the safety comment accordingly: the lock-free read is valid only for array-registered caches, and must not be used on _runtime_stubs without that cache's lock. - NativeFunc::create() now calls allocSize() instead of recomputing the size formula, so memoryUsage() and the allocation can't drift. - Reword CodeCacheArray::memoryUsage() to drop the incorrect "reflects symbols added after registration" claim (adds happen pre-publish); it reflects each library's fully-populated state at dump. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y size Address the Sphinx review's test-coverage gaps: - ArrayMemoryUsageSumsLibraries: exercises CodeCacheArray::memoryUsage() (the aggregation production consumes via Profiler::dump()), which no test covered — guards the loop and accumulation. - MemoryUsageCountsDwarfTable: populates a DWARF table via setDwarfTable() so the length * sizeof(FrameDesc) term is non-zero and a regression in it is detectable. - MemoryUsageCountsFullBlobArray: tightened from EXPECT_GT to an exact EXPECT_EQ (blob array + own name) so over-counting would fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The comments on the lock-free memoryUsage() read named Libraries::_runtime_stubs (a vestigial, unused member) as the example of a continuously-mutated unpublished cache. The actual one is JitCodeCache::_runtime_stubs, mutated under JitCodeCache::_stubs_lock (add() under lock(), findRuntimeStub() under lockShared()). Fix the references. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9e64ead to
90523b9
Compare
Addressing review feedback on visibility: _published was a plain bool read/written across threads (set by CodeCacheArray::add(), read by the add()/expand()/setDwarfTable() asserts). Make it std::atomic<bool> with a release store in markPublished() and acquire loads in the asserts. The store is sequenced before add()'s RELEASE publish of the pointer, so any thread that reaches a published cache observes _published == true — now without a formal data race and TSan-clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Good catch — made |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
ddprof-lib/src/main/cpp/codeCache.cpp:25
- NativeFunc::allocSize() returns 0 for nullptr, but NativeFunc::create() now unconditionally calls allocSize(name) and then does aligned_alloc/strcpy. If a null name ever reaches create(), this becomes undefined behavior (size==0 allocation and strcpy(nullptr)). Add an assertion (or explicit null handling) so the contract is enforced locally.
char *NativeFunc::create(const char *name, short lib_index) {
size_t size = allocSize(name);
NativeFunc *f = (NativeFunc *)aligned_alloc(sizeof(NativeFunc*), size);
f->_lib_index = lib_index;
f->_mark = 0;
// cppcheck-suppress memleak
return strcpy(f->_name, name);
}
copyFrom() memcpy'd the CodeBlob array, which copied each blob's _name *pointer* — so a copied CodeCache shared name allocations with the original and both destructors freed them (double-free / use-after-free). Give the copy its own name strings via NativeFunc::create(), matching how build-id and the DWARF table are already deep-copied. (CodeCache is only ever used by pointer today, so the copy path is currently unexercised — this hardens it rather than fixing an observed crash.) Adds CopyIsDeepAndDoesNotDoubleFree, which copies a populated cache and destroys both, aborting on the old shallow-copy behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Fixed in the latest commit — |
…ootprint" CodeCache::memoryUsage() is a capacity/count-based estimate on main, so "live heap footprint" overstated it. Reword to "approximate heap usage" and note #677 makes memoryUsage() exact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Benchmark Results (commit 9800ad3)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126276045 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10161 ms (21 iters) | ✅ 10263 ms (21 iters) | ≈ +1% (±12%) | — / — |
| akka-uct | 25 | ✅ 8973 ms (24 iters) | ✅ 8882 ms (24 iters) | ≈ -1% (±9.7%) | — / — |
| finagle-chirper | 21 | ✅ 6032 ms (33 iters) | ✅ 5973 ms (33 iters) | ≈ -1% (±26%) | |
| finagle-chirper | 25 | ✅ 5508 ms (36 iters) | ✅ 5482 ms (36 iters) | ≈ -0.5% (±23.8%) | |
| fj-kmeans | 21 | ✅ 2709 ms (70 iters) | ✅ 2763 ms (68 iters) | ≈ +2% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2860 ms (66 iters) | ✅ 2828 ms (66 iters) | ≈ -1.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2094 ms (88 iters) | ✅ 2098 ms (89 iters) | ≈ +0.2% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2013 ms (92 iters) | ✅ 2127 ms (87 iters) | 🔴 +5.7% | — / — |
| naive-bayes | 21 | ✅ 1259 ms (136 iters) | ✅ 1253 ms (137 iters) | ≈ -0.5% (±32.3%) | — / — |
| naive-bayes | 25 | ✅ 1020 ms (168 iters) | ✅ 1016 ms (169 iters) | ≈ -0.4% (±31.4%) | — / — |
| reactors | 21 | ✅ 16733 ms (15 iters) | ✅ 16874 ms (15 iters) | ≈ +0.8% (±6.1%) | — / — |
| reactors | 25 | ✅ 18576 ms (15 iters) | ✅ 18322 ms (15 iters) | ≈ -1.4% (±5.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 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2064 / 1929 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2232 / 2122 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 8913 / 8489 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8849 / 8204 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 5 / ✅ | 1278 / 1253 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1306 / 1253 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2939 / 3027 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2906 / 2868 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 3536 / 3548 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 3471 / 3479 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1668 / 1709 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1904 / 1846 | ✅ / ✅ | ✅ / ✅ |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/126276036 |
…ootprint" CodeCache::memoryUsage() is a capacity/count-based estimate on main, so "live heap footprint" overstated it. Reword to "approximate heap usage" and note #677 makes memoryUsage() exact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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).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, and the DWARF table.Libraries::updateBuildIds) frees and replaces_build_idon already-published caches under_build_id_lock, which dump does not hold, so dereferencing it here (strlen) would race → use-after-free. It is negligible (~tens of bytes per library) next to the symbol tables, so excluding it keeps this read lock-free at no meaningful accuracy cost.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.CodeCacheArray(their_blobs/DWARF fields are fixed once published). This is now enforced: a_publishedflag is set byCodeCacheArray::add(), andadd()/expand()/setDwarfTable()assert!_published(debug builds). Standalone, continuously-mutated caches likeLibraries::_runtime_stubsare never published and are exempt.NativeFunc::create()callsNativeFunc::allocSize()so the allocation size andmemoryUsage()'s accounting share one formula and can't drift.memoryUsage()is only called at dump time, so theO(symbols)recompute is off any hot path.Testing
codeCache_ut(5 tests): per-symbol name length tracked exactly; longer name costs more; empty-cache size is exactly blob-array + own-name (EXPECT_EQ); DWARF table term counted; andCodeCacheArray::memoryUsage()sums the per-library values (the aggregationProfiler::dump()actually consumes).:ddprof-lib:gtestDebugsuite green — 354 tests, 0 failures (rebased on latestmain).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