fix(profiler): complete malloc/native-socket hook-prefix-skip stackwalking#648
Draft
jbachorik wants to merge 7 commits into
Draft
fix(profiler): complete malloc/native-socket hook-prefix-skip stackwalking#648jbachorik wants to merge 7 commits into
jbachorik wants to merge 7 commits into
Conversation
…ix missing native frames The event name/category did not follow the datadog.* convention used by other events. Separately, Profiler::getNativeTrace unconditionally skipped native stack unwinding for BCI_NATIVE_MALLOC under the default cstack setting, leaving only Java frames in malloc call stacks.
Same guard in Profiler::getNativeTrace that was skipping native unwinding for BCI_NATIVE_MALLOC also applied to BCI_NATIVE_SOCKET; both are non-CPU/WALL event types and were bypassed when cstack is left at its default.
Marks the socket hook wrapper symbols (send/recv/write/read_hook) with MARK_ASYNC_PROFILER, mirroring the existing malloc hook marking, and teaches convertNativeTrace to discard frames captured before reaching that mark instead of truncating the stack there, so only the real caller frames remain for BCI_NATIVE_MALLOC/BCI_NATIVE_SOCKET traces.
Adds SOCKET_SAMPLE to EventType so BCI_NATIVE_SOCKET gets its own identity in walkVM, and fixes a bug where the generic is_marked check in HotspotSupport::walkVM terminated the scan on ANY mark before the MARK_ASYNC_PROFILER dispatch below it could ever run, making the malloc hook-skip logic dead code. NativeFrameResolution now carries the actual mark value so walkVM can dispatch correctly: MARK_ASYNC_PROFILER resets depth and resumes past the hook boundary for malloc/socket samples (matching the FP/DWARF path), other marks still terminate the scan as before.
Surface silent hook-mark resolution failures via counters/logging, make convertNativeTrace's MARK_ASYNC_PROFILER reset gate on skip_hook_prefix instead of the mutable skipping flag, restore the dropped MARK_COMPILER_ENTRY dispatch in walkVM, drop the now-redundant is_marked field, de-duplicate the hook-prefixed-sample predicate, and add stack trace assertions for the socket hook boundary skip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
CI Test ResultsRun: #29273352717 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 17-j9Job: View logs No detailed failure information available. Check the job logs. glibc-amd64/debug / 11-j9Job: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 30 | Failed: 2 Updated: 2026-07-13 18:28:31 UTC |
jbachorik
changed the title
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
Contributor
Benchmark Results (commit 15f3132)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124275785 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10394 ms (21 iters) | ✅ 10298 ms (21 iters) | ≈ -0.9% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8826 ms (24 iters) | ✅ 8991 ms (24 iters) | ≈ +1.9% (±10.3%) | — / — |
| finagle-chirper | 21 | ✅ 6006 ms (33 iters) | ✅ 5950 ms (33 iters) | ≈ -0.9% (±24.8%) | |
| finagle-chirper | 25 | ✅ 5496 ms (36 iters) | ✅ 5422 ms (36 iters) | ≈ -1.3% (±23.9%) | |
| fj-kmeans | 21 | ✅ 2714 ms (69 iters) | ✅ 2695 ms (69 iters) | ≈ -0.7% (±2.8%) | — / — |
| fj-kmeans | 25 | ✅ 2810 ms (67 iters) | ✅ 2810 ms (66 iters) | ≈ 0% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2113 ms (88 iters) | ✅ 2049 ms (91 iters) | 🟢 -3% | — / — |
| future-genetic | 25 | ✅ 2070 ms (90 iters) | ✅ 2122 ms (87 iters) | 🔴 +2.5% | — / — |
| naive-bayes | 21 | ✅ 1253 ms (136 iters) | ✅ 1275 ms (134 iters) | ≈ +1.8% (±32.9%) | — / — |
| naive-bayes | 25 | ✅ 982 ms (174 iters) | ✅ 1016 ms (168 iters) | ≈ +3.5% (±32.7%) | — / — |
| reactors | 21 | ✅ 16060 ms (15 iters) | ✅ 16114 ms (15 iters) | ≈ +0.3% (±8.8%) | — / — |
| reactors | 25 | ✅ 18976 ms (15 iters) | ✅ 18213 ms (15 iters) | ≈ -4% (±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 | 2044 / 2050 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 6 | 2480 / 2333 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8280 / 8924 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8266 / 8237 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1276 / 1261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1288 / 1276 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 3037 / 3032 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2854 / 2928 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 3486 / 3500 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 4 | 3445 / 3496 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1539 / 1646 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1889 / 1884 | ✅ / ✅ | ✅ / ✅ |
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124386433 |
Contributor
Benchmark Results (commit d27a19e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124386442 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10361 ms (21 iters) | ✅ 10194 ms (21 iters) | ≈ -1.6% (±11.5%) | — / — |
| akka-uct | 25 | ✅ 8822 ms (24 iters) | ✅ 8881 ms (24 iters) | ≈ +0.7% (±10%) | — / — |
| finagle-chirper | 21 | ✅ 5917 ms (33 iters) | ✅ 6021 ms (33 iters) | ≈ +1.8% (±25.6%) | |
| finagle-chirper | 25 | ✅ 5469 ms (36 iters) | ✅ 5514 ms (36 iters) | ≈ +0.8% (±24.8%) | |
| fj-kmeans | 21 | ✅ 2644 ms (72 iters) | ✅ 2783 ms (67 iters) | 🔴 +5.3% | — / — |
| fj-kmeans | 25 | ✅ 2843 ms (66 iters) | ✅ 2848 ms (66 iters) | ≈ +0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2063 ms (90 iters) | ✅ 2149 ms (86 iters) | 🔴 +4.2% | — / — |
| future-genetic | 25 | ✅ 2024 ms (92 iters) | ✅ 2093 ms (88 iters) | 🔴 +3.4% | — / — |
| naive-bayes | 21 | ✅ 1269 ms (135 iters) | ✅ 1297 ms (132 iters) | ≈ +2.2% (±32.6%) | — / — |
| naive-bayes | 25 | ✅ 1014 ms (169 iters) | ✅ 1009 ms (169 iters) | ≈ -0.5% (±31.9%) | — / — |
| reactors | 21 | ✅ 17474 ms (15 iters) | ✅ 16021 ms (15 iters) | 🟢 -8.3% | — / — |
| reactors | 25 | ✅ 18483 ms (15 iters) | ✅ 18350 ms (15 iters) | ≈ -0.7% (±5.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 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2004 / 2048 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 2226 / 2232 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 7951 / 8195 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8519 / 8278 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1280 / 1269 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 4 | 1289 / 1281 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 4 / ✅ | 2931 / 2913 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2772 / 2826 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3493 / 3499 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 1 | 3516 / 3479 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1786 / 1703 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1880 / 1796 | ✅ / ✅ | ✅ / ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
Fixes up gaps in the malloc/native-socket hook-prefix-skip stackwalking logic introduced earlier on this branch (
convertNativeTraceandHotspotSupport::walkVM), which was incomplete:markAsyncProfilerHooknow returnsbool;NativeSocketSampler::start()checks it).MARK_ASYNC_PROFILERhook boundary is never found in a callchain, so a fully-discarded/empty native stack is observable rather than silent.convertNativeTrace'sMARK_ASYNC_PROFILERreset to gate on the immutableskip_hook_prefixparameter instead of the mutableskippingflag, so a second boundary-marked frame later in the same callchain is still treated as a reset point (matcheswalkVM's per-frame dispatch).MARK_COMPILER_ENTRYdispatch branch inwalkVMthat had been dropped without updating thecomp_taskflag/docs that still reference it.is_markedfield onNativeFrameResolutionin favor of a computedmark != 0accessor, and updates the stale doc comments describing per-mark dispatch semantics.isHookPrefixedSample()helper (BCI and EventType overloads) used at all 4 call sites that previously hardcoded the "is this a malloc/socket sample" predicate independently.NativeSocketStackTraceTestproving a real caller frame (not a profiler-internal hook frame) is present after unwinding.Motivation:
The hook-prefix-skip stackwalking added earlier on this branch was incomplete — this closes the gaps before the branch merges.
Additional Notes:
The restored
MARK_COMPILER_ENTRYdispatch is currently unreachable:_features.comp_taskis never set to1anywhere in the codebase (no CLI flag wires it), so this fix restores doc/dead-code consistency but doesn't change observable behavior. Revisiting that separately.How to test the change?:
./gradlew assembleDebugJarbuilds clean;./gradlew compileTestJavacompiles clean. The new/updated assertions inNativeSocketStackTraceTestare parameterized acrossvm/vmx/dwarf/fpcstack modes and cover the fixed code paths.For 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!