feat(profiler): native malloc/socket stack capture with profiler-hook frame skipping#648
feat(profiler): native malloc/socket stack capture with profiler-hook frame skipping#648jbachorik wants to merge 13 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>
CI Test ResultsRun: #29911564272 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 25-graalJob: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-07-22 11:47:43 UTC |
jbachorik
changed the title
|
@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". |
|
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 | ✅ / ✅ | ✅ / ✅ |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124386433 |
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 | ✅ / ✅ | ✅ / ✅ |
Benchmark Results (commit bc3ec7d)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124681069 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10229 ms (21 iters) | ✅ 10257 ms (21 iters) | ≈ +0.3% (±11.7%) | — / — |
| akka-uct | 25 | ✅ 8894 ms (24 iters) | ✅ 8893 ms (24 iters) | ≈ -0% (±10.2%) | — / — |
| finagle-chirper | 21 | ✅ 5935 ms (33 iters) | ✅ 5986 ms (33 iters) | ≈ +0.9% (±25.8%) | |
| finagle-chirper | 25 | ✅ 5454 ms (36 iters) | ✅ 5481 ms (36 iters) | ≈ +0.5% (±24.4%) | |
| fj-kmeans | 21 | ✅ 2749 ms (68 iters) | ✅ 2766 ms (67 iters) | ≈ +0.6% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2803 ms (66 iters) | ✅ 2832 ms (66 iters) | ≈ +1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2048 ms (90 iters) | ✅ 2070 ms (89 iters) | ≈ +1.1% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2003 ms (92 iters) | ✅ 2077 ms (89 iters) | 🔴 +3.7% | — / — |
| naive-bayes | 21 | ✅ 1249 ms (137 iters) | ✅ 1286 ms (133 iters) | ≈ +3% (±33.7%) | — / — |
| naive-bayes | 25 | ✅ 1002 ms (171 iters) | ✅ 1016 ms (168 iters) | ≈ +1.4% (±31.4%) | — / — |
| reactors | 21 | ✅ 16793 ms (15 iters) | ✅ 16897 ms (15 iters) | ≈ +0.6% (±6.4%) | — / — |
| reactors | 25 | ✅ 18363 ms (15 iters) | ✅ 18376 ms (15 iters) | ≈ +0.1% (±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 | 1991 / 2023 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2163 / 2366 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 8457 / 8255 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 8831 / 8311 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 1296 / 1255 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 1260 / 1271 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2950 / 2924 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2839 / 2915 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 10 | 3524 / 3505 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 1 | 3471 / 3494 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1701 / 1691 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1839 / 1836 | ✅ / ✅ | ✅ / ✅ |
Benchmark Results (commit d061e9a)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125889277 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10242 ms (21 iters) | ✅ 10393 ms (21 iters) | ≈ +1.5% (±11.7%) | — / — |
| akka-uct | 25 | ✅ 8872 ms (24 iters) | ✅ 8832 ms (24 iters) | ≈ -0.5% (±10.5%) | — / — |
| finagle-chirper | 25 | ✅ 5503 ms (36 iters) | ✅ 5447 ms (36 iters) | ≈ -1% (±24%) | |
| fj-kmeans | 21 | ✅ 2758 ms (67 iters) | ✅ 2745 ms (68 iters) | ≈ -0.5% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2829 ms (66 iters) | ✅ 2827 ms (66 iters) | ≈ -0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2140 ms (87 iters) | ✅ 2052 ms (91 iters) | 🟢 -4.1% | — / — |
| future-genetic | 25 | ✅ 2049 ms (92 iters) | ✅ 1975 ms (94 iters) | 🟢 -3.6% | — / — |
| naive-bayes | 25 | ✅ 1034 ms (165 iters) | ✅ 1024 ms (167 iters) | ≈ -1% (±31.6%) | — / — |
| reactors | 21 | ✅ 16809 ms (15 iters) | ✅ 15992 ms (15 iters) | ≈ -4.9% (±6.9%) | — / — |
| reactors | 25 | ✅ 18306 ms (15 iters) | ✅ 18481 ms (15 iters) | ≈ +1% (±4.9%) | — / — |
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 / 1 | 1967 / 2019 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 6 / 3 | 2358 / 2103 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 8340 / 8558 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 8331 / 8123 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 5 | 1256 / 1255 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 5 | 1279 / 1251 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 4 | 2965 / 3033 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2898 / 2829 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 3491 / 3504 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 5 | 3500 / 3493 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1755 / 1655 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1897 / 1883 | ✅ / ✅ | ✅ / ✅ |
Benchmark Results (commit d669181)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125904596 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10272 ms (21 iters) | ✅ 10242 ms (21 iters) | ≈ -0.3% (±11.1%) | — / — |
| akka-uct | 25 | ✅ 8962 ms (24 iters) | ✅ 8803 ms (24 iters) | ≈ -1.8% (±10.7%) | — / — |
| finagle-chirper | 21 | ✅ 6007 ms (33 iters) | ✅ 5965 ms (33 iters) | ≈ -0.7% (±25%) | |
| finagle-chirper | 25 | ✅ 5466 ms (36 iters) | ✅ 5520 ms (36 iters) | ≈ +1% (±24.4%) | |
| fj-kmeans | 21 | ✅ 2710 ms (69 iters) | ✅ 2648 ms (71 iters) | ≈ -2.3% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2830 ms (66 iters) | ✅ 2821 ms (66 iters) | ≈ -0.3% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2117 ms (88 iters) | ✅ 2159 ms (86 iters) | ≈ +2% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2041 ms (91 iters) | ✅ 2120 ms (88 iters) | 🔴 +3.9% | — / — |
| naive-bayes | 21 | ✅ 1268 ms (135 iters) | ✅ 1234 ms (137 iters) | ≈ -2.7% (±32%) | — / — |
| naive-bayes | 25 | ✅ 1022 ms (167 iters) | ✅ 1008 ms (170 iters) | ≈ -1.4% (±30.9%) | — / — |
| reactors | 21 | ✅ 15764 ms (15 iters) | ✅ 16111 ms (16 iters) | ≈ +2.2% (±7.7%) | — / — |
| reactors | 25 | ✅ 18555 ms (15 iters) | ✅ 18451 ms (15 iters) | ≈ -0.6% (±4.3%) | — / — |
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 / 2 | 1947 / 1993 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 2303 / 2295 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 8615 / 8656 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 8520 / 8206 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1277 / 1247 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1271 / 1266 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 4 / ✅ | 2980 / 2989 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2909 / 2987 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 8 / 4 | 3535 / 3496 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3506 / 3503 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1593 / 1651 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1885 / 1816 | ✅ / ✅ | ✅ / ✅ |
![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: fb32d57629
ℹ️ 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".
Benchmark Results (commit fb32d57)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125918579 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10282 ms (21 iters) | ✅ 10328 ms (21 iters) | ≈ +0.4% (±11.6%) | — / — |
| akka-uct | 25 | ✅ 8786 ms (24 iters) | ✅ 8868 ms (24 iters) | ≈ +0.9% (±10.4%) | — / — |
| finagle-chirper | 21 | ✅ 5939 ms (33 iters) | ✅ 5975 ms (33 iters) | ≈ +0.6% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5421 ms (36 iters) | ✅ 5493 ms (36 iters) | ≈ +1.3% (±24.8%) | |
| fj-kmeans | 21 | ✅ 2766 ms (68 iters) | ✅ 2688 ms (70 iters) | 🟢 -2.8% | — / — |
| fj-kmeans | 25 | ✅ 2834 ms (66 iters) | ✅ 2832 ms (66 iters) | ≈ -0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2067 ms (89 iters) | ✅ 2123 ms (88 iters) | 🔴 +2.7% | — / — |
| future-genetic | 25 | ✅ 2074 ms (89 iters) | ✅ 2004 ms (93 iters) | 🟢 -3.4% | — / — |
| naive-bayes | 21 | ✅ 1248 ms (136 iters) | ✅ 1273 ms (134 iters) | ≈ +2% (±33%) | — / — |
| naive-bayes | 25 | ✅ 1028 ms (167 iters) | ✅ 952 ms (179 iters) | ≈ -7.4% (±30.3%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 1946 / 2044 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2395 / 2209 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 8382 / 8478 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8226 / 8413 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1277 / 1284 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 1299 / 1263 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2868 / 2926 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 6 / ✅ | 2850 / 2904 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 8 / 3 | 3509 / 3466 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 3497 / 3466 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1903 / 1862 | ✅ / ✅ | ✅ / ✅ |
|
Before we start reviewing, I think the PR title/description need to be brought in line with what actually lands on
The bullet-by-bullet breakdown itself is great; this is just a re-scope + rename, not a rewrite. |
|
Sorry, wanted to click on 'Update branch' and did 'Ready for review' before it was actually ready :/ |
jbachorik
changed the title
jbachorik
changed the title
|
@rkennke The PR should be ready now |
|
Thanks for the JIRA link and the event-rename bullet. Two of the three asks are still open though, and both are quick — here's concrete text so it's copy-paste: Ask #2 — title prefix. Change:
to something like:
(net effect on Ask #3 — stale marker name. The tip commit renames Ask #1 — framing (partially done). The new event-rename bullet helps, but the opening still reads as a follow-up. Suggest reframing the intro from:
to something that leads with what the PR delivers, e.g.:
...then keep the existing bullet list as the detailed breakdown. Same content, just scoped to what lands on |
jbachorik
changed the title
|
@rkennke 🤞 all PR desc changes are in |
rkennke
left a comment
rkennke
left a comment
There was a problem hiding this comment.
Reviewed the net diff against main. The logic is sound — I traced both unwinding paths and the central fix is real: on main, walkVM's if (resolution.is_marked) break; ran before the mark-dispatch chain, making the whole MARK_* dispatch dead code (every marked frame terminated the scan). The new per-mark dispatch with else { break; } fixes that while keeping VM_RUNTIME/INTERPRETER as terminators. convertNativeTrace gating the reset on the immutable skip_hook_prefix is correct and now matches walkVM. The socket test is a genuine positive+negative assertion (real caller present, no *_hook leak), parameterized across cstack modes.
One intentional behavior change worth flagging in the description: the walkVM MARK_JAVA_PROFILER path now does depth = 0; goto dwarf_unwind;, which skips the fillFrame that previously emitted the hook frame — so the hook frame is now dropped for malloc under vm/vmx too (not just socket). Deliberate and consistent, just wire-visible for existing malloc consumers.
Inline notes below — all minor; #1 and #3 are the ones I'd actually action.
| hooks_marked &= markAsyncProfilerHook((void*)&NativeSocketSampler::recv_hook); | ||
| hooks_marked &= markAsyncProfilerHook((void*)&NativeSocketSampler::write_hook); | ||
| hooks_marked &= markAsyncProfilerHook((void*)&NativeSocketSampler::read_hook); | ||
| if (!hooks_marked) { |
There was a problem hiding this comment.
Coverage gap (acceptable): nothing exercises this failure branch or the new NATIVE_HOOK_MARK_RESOLVE_FAILED / NATIVE_TRACE_HOOK_PREFIX_NOT_FOUND counters. Genuinely hard to unit-test, so just noting it rather than requesting a test.
…ename markAsyncProfilerHook, fix stale comment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bits found no code fix to apply🟢 Investigated · ⚪ No code fix needed No actionable CI failures were found for this PR, so no code change was made. View in Datadog | Reviewed commit 8e3bb25 · Any feedback? Reach out in #deveng-pr-agent |
What does this PR do?:
Adds native stack capture for malloc (
BCI_NATIVE_MALLOC) and native-socket (BCI_NATIVE_SOCKET) samples, stripping profiler-internal hook frames so only real caller frames remain — across all cstack modes (fp/dwarf/vm/vmx). Also renames the JFR event todatadog.NativeMemoryAllocation.markAsyncProfilerHooknow returnsbool;NativeSocketSampler::start()checks it).MARK_JAVA_PROFILERhook boundary is never found in a callchain, so a fully-discarded/empty native stack is observable rather than silent.convertNativeTrace'sMARK_JAVA_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.profiler.MallocJFR event todatadog.NativeMemoryAllocationand moves it into the Datadog/Profiling JFR category.Motivation:
Native malloc/socket samples currently include profiler-internal hook frames (and, in some cstack modes, drop the real caller entirely), which pollutes symbolication and makes the resulting stacks hard to read.
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!