Make ProfiledThread's UnwindFailures field debug only - #734
Conversation
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||
CI Test ResultsRun: #31523867699 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-08-11 19:01:35 UTC |
There was a problem hiding this comment.
Pull request overview
This PR makes unwind-failure tracking (the UnwindFailures counters surfaced via the datadog.UnwindFailure JFR event) available only in DEBUG-instrumented native builds, reducing per-thread memory overhead in release builds while keeping the diagnostics pipeline intact for debug/asan/tsan.
Changes:
- Compile-gate
unwindStats.{h,cpp}and per-threadProfiledThread::_unwind_failuresbehindDEBUG, introducing adebug_only(...)macro to simplify conditional inclusion. - Guard unwind-failure emission/serialization paths in HotSpot VM unwinding (
HotspotSupport::walkVM) and JFR writing (Recording::writeUnwindFailures) to DEBUG builds. - Add a JUnit smoke test that validates
datadog.UnwindFailureevent schema when present, and update the unwind-failures benchmark to compile with-DDEBUG.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ddprof-test/src/test/java/com/datadoghq/profiler/UnwindFailuresTest.java | Adds a DEBUG-only smoke test for the datadog.UnwindFailure JFR event schema under cstack=vm. |
| ddprof-lib/src/test/cpp/ddprof_ut.cpp | Wraps UnwindFailures/UnwindStats unit tests in #ifdef DEBUG. |
| ddprof-lib/src/main/cpp/unwindStats.h | Makes UnwindFailures/UnwindStats definitions DEBUG-only. |
| ddprof-lib/src/main/cpp/unwindStats.cpp | Makes UnwindStats static member definitions DEBUG-only. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Makes ProfiledThread’s per-thread UnwindFailures storage and accessor DEBUG-only. |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | Records unwind failures only in DEBUG builds during walkVM unwinding. |
| ddprof-lib/src/main/cpp/flightRecorder.cpp | Writes datadog.UnwindFailure events only in DEBUG builds. |
| ddprof-lib/src/main/cpp/common.h | Introduces debug_only(...) macro for DEBUG-conditional code. |
| ddprof-lib/benchmarks/build.gradle.kts | Compiles the unwind-failures benchmark with -DDEBUG. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7ec37a9d9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return 0; | ||
| } | ||
|
|
||
| debug_only(UnwindFailures* unwindFailures = prof_thread->unwindFailures();) |
There was a problem hiding this comment.
Avoid resetting unwind stats on every sample
In DEBUG/ASan/TSan builds with cstack=vm, every CPU/wall sample reaches this new call and unwindFailures() defaults to reset=true, so it invokes clear() even when no unwind failure has occurred. That clear() zeros the 1024-name/counter buffers (about 288 KiB) on the signal sampling path for each walk, which can dominate debug/sanitizer profiling; keep the per-thread buffer untouched until a failure is recorded or make the reset O(1).
AGENTS.md reference: AGENTS.md:L375-L376
Useful? React with 👍 / 👎.
| # endif | ||
| #endif | ||
|
|
||
| #ifdef DEBUG |
There was a problem hiding this comment.
Add the required Datadog copyright header
This modified header still starts directly with the include guard, but the repository instructions require adding a Datadog copyright when touching files that do not already contain one. The same issue applies to the modified unwindStats.h and unwindStats.cpp files, so please add the standard current-year header before landing.
AGENTS.md reference: AGENTS.md:L372-L374
Useful? React with 👍 / 👎.
What does this PR do?:
Removes the UnwindFailures/UnwindStats per-thread diagnostic counters from release builds, gating them entirely behind #ifdef DEBUG, and backfills a test.
New macro — debug_only(s) expands to s when DEBUG is defined, else to nothing.
threadLocalData.h
ProfiledThread::_unwind_failuresis now debug_only(...), and the unwindFailures() accessor is wrapped in #ifdef DEBUG. This is the actual memory-overhead fix — the field no longer exists per-thread on release builds.unwindStats.h: entire UnwindStats class and its static members wrapped in #ifdef DEBUG — the whole subsystem is compiled out on release.
-flightRecorder.cpp: Recording::writeUnwindFailures() body gated by #ifdef DEBUG (becomes a no-op on release).
hotspotSupport.cpp: the call sites that record into UnwindFailures are gated by #ifdef DEBUG.
build.gradle.kts: the unwindFailuresBenchmark now compiles with -DDEBUG, since the code it benchmarks no longer exists without that define.
Tests:
-- existing UnwindFailures/UnwindStats gtest cases in ddprof_ut.cpp wrapped in #ifdef DEBUG;
-- new UnwindFailuresTest.java — a JFR smoke test under cstack=vm that skips unless the build config is debug/*san, asserts the profiler runs cleanly, and sanity-checks datadog.UnwindFailure event schema if any happen to fire (a real stub-unwind failure is described as too rare to force deterministically).
Motivation:
Reduce profiler memory usage in release build
Additional Notes:
How to test the change?:
UnwindFailuresin debug buildFor Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!