fix(codecache): make memoryUsage() accurate and live by rkennke · Pull Request #677 · DataDog/java-profiler

rkennke · 2026-07-21T11:03:54Z

What

Makes CodeCache::memoryUsage() — which drives the CODECACHE_NATIVE_SIZE_BYTES counter — report the profiler's actual native-symbol memory, instead of a stale, heavily-undercounted approximation.

(CodeCache here 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:

Blob array undercounted ~3× — counted sizeof(CodeBlob*) (a pointer, 8 B) where the array actually holds CodeBlob (three pointers, 24 B).
Symbol names undercounted — approximated as a fixed sizeof(NativeFunc) (4 B) per symbol, ignoring the name-string length that dominates each allocation (aligned_alloc(sizeof(NativeFunc) + 1 + strlen(name))).
Missing entirely — the DWARF unwind table (_dwarf_table, can be large).
Stale — CodeCacheArray cached the sum at add() 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 (new NativeFunc::allocSize()), this cache's own name, and the DWARF table.
The build-id string is intentionally excluded: 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 (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 concurrent add()s. The now-dead _used_memory field and its add()-time accumulation are removed.
The lock-free read is only valid for caches registered in a CodeCacheArray (their _blobs/DWARF fields are fixed once published). This is now enforced: a _published flag is set by CodeCacheArray::add(), and add()/expand()/setDwarfTable() assert !_published (debug builds). Standalone, continuously-mutated caches like Libraries::_runtime_stubs are never published and are exempt.
NativeFunc::create() calls NativeFunc::allocSize() so the allocation size and memoryUsage()'s accounting share one formula and can't drift.
memoryUsage() is only called at dump time, so the O(symbols) recompute is off any hot path.

Testing

New 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; and CodeCacheArray::memoryUsage() sums the per-library values (the aggregation Profiler::dump() actually consumes).
Full :ddprof-lib:gtestDebug suite green — 354 tests, 0 failures (rebased on latest main).

Note

Independent of the native-memory accounting work in #669; that PR's NM_NATIVE_SYMBOLS gauge mirrors this memoryUsage(), so it benefits automatically once both land, but neither depends on the other to build.

🤖 Generated with Claude Code

Copilot

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_memory and sums per-library memoryUsage() at call time to reflect post-registration growth.
Adds codeCache_ut coverage validating that memory usage scales with symbol-name length and counts the full CodeBlob array 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.

dd-octo-sts · 2026-07-21T11:30:00Z

CI Test Results

Run:#29932410242 | Commit:76a7976 | Duration: 14m 32s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-amd64/debug
8	-	✅	-	-
8-ibm	-	✅	-	-
8-j9	✅	✅	-	-
8-librca	-	-	✅	✅
8-orcl	-	✅	-	-
11	-	✅	-	-
11-j9	✅	✅	-	-
11-librca	-	-	✅	✅
17	✅	✅	-	-
17-graal	✅	✅	-	-
17-j9	✅	✅	-	-
17-librca	-	-	✅	✅
21	✅	✅	-	-
21-graal	✅	✅	-	-
21-librca	-	-	✅	✅
25	✅	✅	-	-
25-graal	✅	✅	-	-
25-librca	-	-	✅	✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0

Updated: 2026-07-22 16:00:22 UTC

dd-octo-sts · 2026-07-21T11:35:31Z

Benchmark Results (commit `cf42bb1`)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125921385 Commit: cf42bb101900e542a11975befba2ad446ee2be97

⚠️ Significant outliers

🟢 future-genetic (JDK 25): runtime -3.9% (2088→2006 ms)

Runtime details (per benchmark × JDK)

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%)	⚠️ W:3 / ⚠️ W:3
finagle-chirper	25	✅ 5505 ms (36 iters)	✅ 5373 ms (36 iters)	≈ -2.4% (±24.2%)	⚠️ W:4 / ⚠️ W:3
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

💡 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".

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

dd-octo-sts · 2026-07-21T12:37:40Z

Benchmark Results (commit `8d1c750`)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125932438 Commit: 8d1c750d9fb7130448dc1c9739e821a9f1cc73b2

⚠️ Significant outliers

🔴 fj-kmeans (JDK 21): runtime +4.4% (2701→2820 ms)
🔴 reactors (JDK 21): runtime +7.3% (16180→17360 ms)

Runtime details (per benchmark × JDK)

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%)	⚠️ W:3 / ⚠️ W:3
finagle-chirper	25	✅ 5511 ms (36 iters)	✅ 5514 ms (36 iters)	≈ +0.1% (±25%)	⚠️ W:3 / ⚠️ W:3
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	✅ / ✅	✅ / ✅

rkennke · 2026-07-21T13:57:08Z

Thanks for the reviews — addressed in cf512dd:

Build-id data race (codex P2 / Copilot) — Good catch. memoryUsage() no longer reads _build_id. The background refresher (Libraries::updateBuildIds) calls setBuildId() (free + replace) on already-published caches under _build_id_lock, which dump doesn't hold, so strlen(_build_id) could race that free → use-after-free. It's set at most once per lib and is only ~tens of bytes (a hash), negligible next to the symbol tables, so it's excluded rather than synchronized — keeping the read lock-free. The rest of the sum is safe: blob names are fixed once a library is published (same read the symbolication fast path does), and the DWARF contribution reads only the length scalar (no deref).

memoryUsage() should be const (Copilot) — Done, on both the declaration and definition.

Use reinterpret_cast for int→pointer in tests / drop unused <cstring> (Copilot) — Both done.

Re: the benchmark outliers (reactors +7.3%, fj-kmeans +4.4% on one commit; future-genetic -3.9% the other) — these look like run-to-run noise: they're near/within the reported variance bands and point in opposite directions across commits, and memoryUsage() runs only at dump time (off any hot path), so a real few-percent runtime effect isn't plausible from this change.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

dd-octo-sts · 2026-07-21T14:30:49Z

Benchmark Results (commit `cf512dd`)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125969722 Commit: cf512dde50e1b2f2f05058076b7451478e5203a9

✅ Within expected boundaries

No significant runtime deltas (all within run-to-run noise) and no internal-counter outliers.

Runtime details (per benchmark × JDK)

Benchmark	JDK	Latest	Dev	Δ (dev vs latest)	Issues L/D
akka-uct	21	✅ 10352 ms (21 iters)	✅ 10422 ms (21 iters)	≈ +0.7% (±11.1%)	— / —
akka-uct	25	✅ 8912 ms (24 iters)	✅ 8911 ms (24 iters)	≈ -0% (±10.3%)	— / —
finagle-chirper	21	✅ 6014 ms (33 iters)	✅ 5976 ms (33 iters)	≈ -0.6% (±25.3%)	⚠️ W:3 / ⚠️ W:3
finagle-chirper	25	✅ 5525 ms (36 iters)	✅ 5447 ms (36 iters)	≈ -1.4% (±24.4%)	⚠️ W:3 / ⚠️ W:3
fj-kmeans	21	✅ 2712 ms (70 iters)	✅ 2769 ms (67 iters)	≈ +2.1% (±2.8%)	— / —
fj-kmeans	25	✅ 2746 ms (68 iters)	✅ 2716 ms (68 iters)	≈ -1.1% (±2.8%)	— / —
future-genetic	21	✅ 2109 ms (88 iters)	✅ 2055 ms (90 iters)	≈ -2.6% (±2.6%)	— / —
future-genetic	25	✅ 2081 ms (90 iters)	✅ 2087 ms (89 iters)	≈ +0.3% (±2.5%)	— / —
naive-bayes	21	✅ 1273 ms (134 iters)	✅ 1279 ms (134 iters)	≈ +0.5% (±32.9%)	— / —
naive-bayes	25	✅ 1021 ms (167 iters)	✅ 1016 ms (168 iters)	≈ -0.5% (±31.5%)	— / —
reactors	21	✅ 16757 ms (15 iters)	✅ 15606 ms (15 iters)	≈ -6.9% (±7.1%)	— / —
reactors	25	✅ 18306 ms (15 iters)	✅ 18114 ms (15 iters)	≈ -1% (±5.4%)	— / —

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	✅ / ✅	✅ / ✅	1 / 3	1968 / 1907	✅ / ✅	✅ / ✅
akka-uct	25	✅ / ✅	✅ / ✅	3 / 1	2398 / 2299	✅ / ✅	✅ / ✅
finagle-chirper	21	✅ / ✅	✅ / ✅	5 / 6	8527 / 8398	✅ / ✅	✅ / ✅
finagle-chirper	25	✅ / ✅	✅ / ✅	2 / ✅	8450 / 8413	✅ / ✅	✅ / ✅
fj-kmeans	21	✅ / ✅	✅ / ✅	1 / 1	1280 / 1250	✅ / ✅	✅ / ✅
fj-kmeans	25	✅ / ✅	✅ / ✅	1 / 1	1251 / 1240	✅ / ✅	✅ / ✅
future-genetic	21	✅ / ✅	✅ / ✅	2 / 1	2907 / 2970	✅ / ✅	✅ / ✅
future-genetic	25	✅ / ✅	✅ / ✅	3 / 1	2956 / 2989	✅ / ✅	✅ / ✅
naive-bayes	21	✅ / ✅	✅ / ✅	5 / 5	3508 / 3527	✅ / ✅	✅ / ✅
naive-bayes	25	✅ / ✅	✅ / ✅	5 / 9	3457 / 3444	✅ / ✅	✅ / ✅
reactors	21	✅ / ✅	✅ / ✅	1 / 1	1710 / 1549	✅ / ✅	✅ / ✅
reactors	25	✅ / ✅	✅ / ✅	✅ / 3	1901 / 1731	✅ / ✅	✅ / ✅

rkennke · 2026-07-22T10:37:48Z

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 updateBuildIds free/replace race (matching the header and code comments). Description and implementation now agree.

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.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

zhengyu123

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());

rkennke · 2026-07-22T12:45:25Z

Thanks for the review!

On the unrelated note (profiler.cpp:1739-1741 calling native_libs.memoryUsage() per counter) — that's already addressed in #669, which factors those lines into a updateNativeLibMemStats() helper that reads memoryUsage() once and reuses it. (#677 doesn't touch profiler.cpp, so nothing to change here.)

Separately, while there: CODECACHE_RUNTIME_STUBS_SIZE_BYTES is set to the same native_libs.memoryUsage() as CODECACHE_NATIVE_SIZE_BYTES — looks like it may be intended to report a distinct quantity (the runtime-stubs cache). I've left the value as-is in #669 to avoid changing that counter's meaning without knowing the intent; happy to follow up separately if it should differ.

rkennke · 2026-07-22T13:16:25Z

Thanks for the thorough Sphinx pass — all seven addressed (cc839e1f0 and the commit before it):

MEDIUM

No test for CodeCacheArray::memoryUsage() — added ArrayMemoryUsageSumsLibraries: builds an array, add()s two populated libs, asserts the aggregate equals the sum of per-lib values.
Safety comment over-claims (_runtime_stubs) — scoped it: the lock-free read is valid only for caches registered in a CodeCacheArray; _runtime_stubs is continuously mutated and unpublished, and must not be read this way without its lock. Reworded the comment accordingly.
allocSize() duplicates create()'s formula — create() now calls allocSize(), single source of truth.
CodeCacheArray::memoryUsage() comment contradicts reality — reworded; dropped the "symbols added after registration" claim (adds happen pre-publish).
Unenforced published-immutability invariant — added a _published flag set by CodeCacheArray::add(); add()/expand()/setDwarfTable() now assert(!_published), so a future post-publish mutation fails loudly in debug. _runtime_stubs (never published) is exempt.

LOW

DWARF term untested — MemoryUsageCountsDwarfTable populates a table via setDwarfTable() and checks the length * sizeof(FrameDesc) contribution.
MemoryUsageCountsFullBlobArray only lower-bounds — tightened to exact EXPECT_EQ (blob array + own name).

Full gtestDebug green (354); the _published asserts don't fire anywhere in the suite, confirming the pre-publish invariant holds on exercised paths (JVM integration paths covered by CI). PR description updated to match.

Copilot

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 calls setBuildId() 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.

rkennke · 2026-07-22T13:26:05Z

Good catch — fixed in the latest commit. My comments named Libraries::_runtime_stubs (which turns out to be an unused/vestigial member); the real continuously-mutated cache is JitCodeCache::_runtime_stubs, guarded by JitCodeCache::_stubs_lock (add() under lock(), findRuntimeStub() under lockShared()). Comments now reference the correct symbol and lock.

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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

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>

rkennke · 2026-07-22T15:06:26Z

Good catch — made _published a std::atomic<bool> (release store in markPublished(), acquire loads in the asserts) in the latest commit. It's set sequenced-before CodeCacheArray::add()'s RELEASE publish of the pointer, so any thread that reaches a published cache (via the array's ACQUIRE load) observes _published == true — now with a proper happens-before and no formal data race (TSan-clean), rather than relying on a plain-bool read.

Copilot

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>

rkennke · 2026-07-22T15:14:17Z

Fixed in the latest commit — copyFrom() now deep-copies each blob's name string (NativeFunc::create()), matching how build-id and the DWARF table were already deep-copied, so a copied CodeCache no longer shares — and double-frees — the originals' name allocations. Added CopyIsDeepAndDoesNotDoubleFree (copies a populated cache, destroys both) which aborts under the old shallow behavior. (Note CodeCache is only ever used by pointer today, so this path was unexercised — the fix is hardening; deleting the copy ops was the alternative, but deep-copy keeps the change surgical.)

…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>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

dd-octo-sts · 2026-07-22T15:44:01Z

Benchmark Results (commit `9800ad3`)

Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/126276045 Commit: 9800ad34e86f24d383d968166f335333306e8d8e

⚠️ Significant outliers

🔴 future-genetic (JDK 25): runtime +5.7% (2013→2127 ms)

Runtime details (per benchmark × JDK)

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%)	⚠️ W:3 / ⚠️ W:3
finagle-chirper	25	✅ 5508 ms (36 iters)	✅ 5482 ms (36 iters)	≈ -0.5% (±23.8%)	⚠️ W:3 / ⚠️ W:3
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	✅ / ✅	✅ / ✅

dd-octo-sts · 2026-07-22T15:49:17Z

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>

…age is exact With #677 merged, CodeCache::memoryUsage() is accurate on main, so the runtime-stubs accessor comment describing it as an approximation (pending #677) is no longer true. Describe current behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…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>

…age is exact With #677 merged, CodeCache::memoryUsage() is accurate on main, so the runtime-stubs accessor comment describing it as an approximation (pending #677) is no longer true. Describe current behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(codecache): report the real runtime-stubs cache size CODECACHE_RUNTIME_STUBS_SIZE_BYTES was set to native_libs.memoryUsage() — the same value as CODECACHE_NATIVE_SIZE_BYTES — a copy-paste since the counter was introduced (d0b0d1e). It never reflected the runtime-stubs cache. The runtime stubs live in JitCodeCache::_runtime_stubs (populated by DynamicCodeGenerated as the JVM emits stubs), a different cache from the native libraries. Add JitCodeCache::runtimeStubsMemoryUsage(), which reads that cache's size under its shared _stubs_lock (the cache is mutated concurrently by the JVMTI callback), and set the counter from it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(codecache): word runtime-stubs usage as approximate, not "live footprint" 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> * docs(codecache): drop stale "approximate/#677" note now that memoryUsage is exact With #677 merged, CodeCache::memoryUsage() is accurate on main, so the runtime-stubs accessor comment describing it as an approximation (pending #677) is no longer true. Describe current behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(codecache): route runtime-stubs usage through JVMSupport The runtime-stubs memory counter called the HotSpot-specific JitCodeCache::runtimeStubsMemoryUsage() directly from shared profiler.cpp, with no VM guard. On J9/Zing that reports a fixed near-zero baseline with nothing in the code marking the value as meaningless there. Route it through the same JVMSupport -> HotspotSupport abstraction the sibling isJitCode() accessor uses: JVMSupport::runtimeStubsMemoryUsage() dispatches to HotspotSupport (-> JitCodeCache) on HotSpot and returns 0 elsewhere. profiler.cpp no longer references JitCodeCache directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(codecache): make memoryUsage() accurate and live 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> * 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) <noreply@anthropic.com> * fix(codecache): don't read build-id in memoryUsage() (data race); review 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> * harden(codecache): enforce/scope the published-immutability invariant 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> * test(codecache): cover array aggregation, DWARF term, exact blob-array 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> * docs(codecache): reference JitCodeCache::_runtime_stubs, not Libraries:: 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> * harden(codecache): make _published atomic (release/acquire) 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> * fix(codecache): deep-copy blob name strings in copyFrom() 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CopilotAI review requested due to automatic review settings July 21, 2026 11:03

Copilotstarted reviewing on behalf of rkennke July 21, 2026 11:04 View session

CopilotAI reviewed Jul 21, 2026
View reviewed changes

Comment threadddprof-lib/src/test/cpp/codeCache_ut.cpp
Comment threadddprof-lib/src/test/cpp/codeCache_ut.cpp Outdated
Comment threadddprof-lib/src/test/cpp/codeCache_ut.cpp Outdated

CopilotAI review requested due to automatic review settings July 21, 2026 12:08

rkennke marked this pull request as ready for review July 21, 2026 12:08

rkennke requested a review from a team as a code owner July 21, 2026 12:08

Copilotstarted reviewing on behalf of rkennke July 21, 2026 12:08 View session

chatgpt-codex-connectorBot reviewed Jul 21, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated

CopilotAI reviewed Jul 21, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated
Comment threadddprof-lib/src/main/cpp/codeCache.h Outdated
Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated

rkennke mentioned this pull request Jul 21, 2026
feat(nativemem): categorized native-memory accounting — first cut #669
Merged

CopilotAI review requested due to automatic review settings July 21, 2026 13:56

Copilotstarted reviewing on behalf of rkennke July 21, 2026 13:57 View session

CopilotAI reviewed Jul 21, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.cpp

CopilotAI review requested due to automatic review settings July 22, 2026 11:02

CopilotAI reviewed Jul 22, 2026

This comment has been minimized.
Sign in to view

zhengyu123 approved these changes Jul 22, 2026
View reviewed changes

CopilotAI review requested due to automatic review settings July 22, 2026 13:15

Copilotstarted reviewing on behalf of rkennke July 22, 2026 13:15 View session

CopilotAI reviewed Jul 22, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.h Outdated
Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated

CopilotAI review requested due to automatic review settings July 22, 2026 13:26

CopilotAI review requested due to automatic review settings July 22, 2026 14:58

rkennke force-pushed the codecache-memusage-accuracy branch from 9e64ead to 90523b9Compare July 22, 2026 14:58

Copilotstarted reviewing on behalf of rkennke July 22, 2026 14:59 View session

jbachorik reviewed Jul 22, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated

CopilotAI reviewed Jul 22, 2026
View reviewed changes

Comment threadddprof-lib/src/main/cpp/codeCache.cpp Outdated

CopilotAI review requested due to automatic review settings July 22, 2026 15:06

Copilotstarted reviewing on behalf of rkennke July 22, 2026 15:06 View session

CopilotAI reviewed Jul 22, 2026
View reviewed changes

CopilotAI review requested due to automatic review settings July 22, 2026 15:14

Copilotstarted reviewing on behalf of rkennke July 22, 2026 15:14 View session

CopilotAI reviewed Jul 22, 2026
View reviewed changes

jbachorik approved these changes Jul 22, 2026
View reviewed changes

rkennke merged commit 69ba264 into mainJul 23, 2026
236 of 237 checks passed

rkennke deleted the codecache-memusage-accuracy branch July 23, 2026 10:24

github-actionsBot added this to the 1.48.0 milestone Jul 23, 2026

kaahos mentioned this pull request Jul 28, 2026
Bump java-profiler (ddprof) to 1.48.0 DataDog/dd-trace-java#12076
Merged

Uh oh!

Conversation

rkennke commented Jul 21, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How

Testing

Note

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dd-octo-stsBot commented Jul 21, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Test Results

Status Overview

Uh oh!

dd-octo-stsBot commented Jul 21, 2026

Benchmark Results (commit cf42bb1)

⚠️ Significant outliers

Uh oh!

chatgpt-codex-connectorBot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dd-octo-stsBot commented Jul 21, 2026

Benchmark Results (commit 8d1c750)

⚠️ Significant outliers

Uh oh!

rkennke commented Jul 21, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

dd-octo-stsBot commented Jul 21, 2026

Benchmark Results (commit cf512dd)

✅ Within expected boundaries

Uh oh!

rkennke commented Jul 22, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

zhengyu123 left a comment

Choose a reason for hiding this comment

Uh oh!

rkennke commented Jul 22, 2026

Uh oh!

rkennke commented Jul 22, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

rkennke commented Jul 22, 2026

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

rkennke commented Jul 21, 2026•
edited
Loading

dd-octo-stsBot commented Jul 21, 2026•
edited
Loading

Benchmark Results (commit `cf42bb1`)

Benchmark Results (commit `8d1c750`)

Benchmark Results (commit `cf512dd`)

Benchmark Results (commit `9800ad3`)