Add compile-time fault-injection infrastructure for stack-walker recovery paths#661
Open
zhengyu123 wants to merge 17 commits into
Open
Add compile-time fault-injection infrastructure for stack-walker recovery paths#661zhengyu123 wants to merge 17 commits into
zhengyu123 wants to merge 17 commits into
Conversation
Contributor
CI Test ResultsRun: #29592251329 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-17 15:51:33 UTC |
Contributor
Benchmark Results (commit d85f055)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124877517 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10232 ms (21 iters) | ✅ 10166 ms (21 iters) | ≈ -0.6% (±11.1%) | — / — |
| akka-uct | 25 | ✅ 8907 ms (24 iters) | ✅ 8929 ms (24 iters) | ≈ +0.2% (±9.6%) | — / — |
| finagle-chirper | 21 | ✅ 5972 ms (33 iters) | ✅ 5984 ms (33 iters) | ≈ +0.2% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5441 ms (36 iters) | ✅ 5530 ms (36 iters) | ≈ +1.6% (±25%) | |
| fj-kmeans | 21 | ✅ 2734 ms (69 iters) | ✅ 2707 ms (70 iters) | ≈ -1% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2822 ms (66 iters) | ✅ 2833 ms (66 iters) | ≈ +0.4% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2088 ms (89 iters) | ✅ 2087 ms (89 iters) | ≈ -0% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2025 ms (92 iters) | ✅ 2137 ms (87 iters) | 🔴 +5.5% | — / — |
| naive-bayes | 21 | ✅ 1286 ms (134 iters) | ✅ 1344 ms (128 iters) | ≈ +4.5% (±33%) | — / — |
| naive-bayes | 25 | ✅ 1010 ms (170 iters) | ✅ 1027 ms (166 iters) | ≈ +1.7% (±32.1%) | — / — |
| reactors | 21 | ✅ 16543 ms (15 iters) | ✅ 16267 ms (15 iters) | ≈ -1.7% (±8%) | — / — |
| reactors | 25 | ✅ 18414 ms (15 iters) | ✅ 18374 ms (15 iters) | ≈ -0.2% (±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 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 2017 / 2006 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2142 / 2423 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 2 | 8761 / 8937 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 8332 / 8785 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1285 / 1288 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1273 / 1302 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 3049 / 3118 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2892 / 2926 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 3506 / 3515 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3476 / 3495 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1758 / 1644 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1913 / 1843 | ✅ / ✅ | ✅ / ✅ |
Contributor
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/125329252 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in, compile-time fault-injection layer (__FAULT_INJECTION__) to deliberately corrupt pointer/value reads at real profiler memory-access sites, so existing recovery paths (SafeAccess safefetch and walkVM setjmp/longjmp) are continuously exercised under test without affecting production builds.
Changes:
- Added new fault-injection infrastructure (
faultInjection.h/.cpp) with tiered probabilities and a per-thread PRNG seed/state. - Wrapped key memory-access sites (stack walker, HotSpot walkVM, VMStructs) with injection macros and added counters to observe injection/recovery activity.
- Wired
-PenableFaultInjectioninto native build configurations and gtest builds; added a dedicated gtest suite for enabled/disabled behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/main/cpp/faultInjection.h | New public fault-injection API/macros gated by __FAULT_INJECTION__. |
| ddprof-lib/src/main/cpp/faultInjection.cpp | New implementation: PRNG, guard-region setup, poison address generation, injected int/long values. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Adds per-thread xorshift64 RNG state and accessors under __FAULT_INJECTION__. |
| ddprof-lib/src/main/cpp/stackWalker.cpp | Wraps SafeAccess dereference sites with fault-injection macros. |
| ddprof-lib/src/main/cpp/hotspot/hotspotSupport.cpp | Wraps selected raw walkVM dereferences; increments longjmp recovery counter. |
| ddprof-lib/src/main/cpp/hotspot/vmStructs.h | Injects faults in VMStructs::at() return pointers (rare tier). |
| ddprof-lib/src/main/cpp/safeAccess.cpp | Adds counter increments on safefetch/safecopy recovery paths. |
| ddprof-lib/src/main/cpp/profiler.cpp | Initializes fault-injection guard region during signal-handler setup (when enabled). |
| ddprof-lib/src/main/cpp/counters.h | Adds new counters for safefetch/safecopy failures, injected faults, and walkVM longjmp recovery. |
| ddprof-lib/src/test/cpp/faultInjection_ut.cpp | New unit tests for disabled identity behavior and enabled fault/recovery behavior. |
| build-logic/conventions/src/main/kotlin/com/datadoghq/native/config/ConfigurationPresets.kt | Adds -D__FAULT_INJECTION__ to debug/release when -PenableFaultInjection is set. |
| build-logic/conventions/src/main/kotlin/com/datadoghq/native/gtest/GtestTaskBuilder.kt | Mirrors the fault-injection define into gtest compilation under -PenableFaultInjection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
Benchmark Results (commit f1c6dc1)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124997579 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):
|
Contributor
Benchmark Results (commit b88f34e)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125033402 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10294 ms (21 iters) | ✅ 10207 ms (21 iters) | ≈ -0.8% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8806 ms (24 iters) | ✅ 8847 ms (24 iters) | ≈ +0.5% (±10.2%) | — / — |
| finagle-chirper | 21 | ✅ 5993 ms (33 iters) | ✅ 5964 ms (33 iters) | ≈ -0.5% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5507 ms (36 iters) | ✅ 5410 ms (36 iters) | ≈ -1.8% (±23.7%) | |
| fj-kmeans | 21 | ✅ 2746 ms (68 iters) | ✅ 2751 ms (68 iters) | ≈ +0.2% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2774 ms (68 iters) | ✅ 2864 ms (66 iters) | 🔴 +3.2% | — / — |
| future-genetic | 21 | ✅ 2097 ms (89 iters) | ✅ 2095 ms (89 iters) | ≈ -0.1% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 2079 ms (89 iters) | ✅ 2142 ms (87 iters) | 🔴 +3% | — / — |
| naive-bayes | 21 | ✅ 1267 ms (135 iters) | ✅ 1274 ms (134 iters) | ≈ +0.6% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1012 ms (169 iters) | ✅ 1007 ms (170 iters) | ≈ -0.5% (±31.2%) | — / — |
| reactors | 21 | ✅ 16315 ms (15 iters) | ✅ 16117 ms (15 iters) | ≈ -1.2% (±7.6%) | — / — |
| reactors | 25 | ✅ 18032 ms (15 iters) | ✅ 18381 ms (15 iters) | ≈ +1.9% (±5.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 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1961 / 1910 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2182 / 2135 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 8807 / 8361 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 8192 / 8209 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1255 / 1265 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 1301 / 1285 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 2941 / 2957 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2834 / 2878 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 3488 / 3503 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1714 / 1536 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1876 / 1844 | ✅ / ✅ | ✅ / ✅ |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…r into zgu/fault-injection
This comment has been minimized.
This comment has been minimized.
Contributor
Benchmark Results (commit a9371e4)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125075940 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10288 ms (21 iters) | ✅ 10290 ms (21 iters) | ≈ +0% (±10.8%) | — / — |
| akka-uct | 25 | ✅ 8894 ms (24 iters) | ✅ 8761 ms (24 iters) | ≈ -1.5% (±10%) | — / — |
| finagle-chirper | 21 | ✅ 5988 ms (33 iters) | ✅ 5998 ms (33 iters) | ≈ +0.2% (±25.4%) | |
| finagle-chirper | 25 | ✅ 5503 ms (36 iters) | ✅ 5442 ms (36 iters) | ≈ -1.1% (±24%) | |
| fj-kmeans | 21 | ✅ 2720 ms (68 iters) | ✅ 2774 ms (68 iters) | ≈ +2% (±2.8%) | — / — |
| fj-kmeans | 25 | ✅ 2832 ms (66 iters) | ✅ 2849 ms (66 iters) | ≈ +0.6% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2044 ms (90 iters) | ✅ 2135 ms (87 iters) | 🔴 +4.5% | — / — |
| future-genetic | 25 | ✅ 2094 ms (89 iters) | ✅ 2020 ms (92 iters) | 🟢 -3.5% | — / — |
| naive-bayes | 21 | ✅ 1328 ms (129 iters) | ✅ 1296 ms (132 iters) | ≈ -2.4% (±32.1%) | — / — |
| naive-bayes | 25 | ✅ 1013 ms (168 iters) | ✅ 987 ms (173 iters) | ≈ -2.6% (±31.4%) | — / — |
| reactors | 21 | ✅ 16133 ms (15 iters) | ✅ 16753 ms (15 iters) | ≈ +3.8% (±6.7%) | — / — |
| reactors | 25 | ✅ 18249 ms (15 iters) | ✅ 18617 ms (15 iters) | ≈ +2% (±4.6%) | — / — |
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 / 2 | 2020 / 1939 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 2250 / 2147 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 8351 / 8601 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 8255 / 8499 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1284 / 1277 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 6 | 1274 / 1293 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2922 / 3030 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2912 / 2860 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 8 / 1 | 3537 / 3554 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 3491 / 3482 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1670 / 1610 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1804 / 1891 | ✅ / ✅ | ✅ / ✅ |
Contributor
Benchmark Results (commit ca8ed89)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125124487 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):
|
Contributor
Benchmark Results (commit c1426c9)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125143187 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10213 ms (21 iters) | ✅ 10164 ms (21 iters) | ≈ -0.5% (±10.7%) | — / — |
| akka-uct | 25 | ✅ 8905 ms (24 iters) | ✅ 8972 ms (24 iters) | ≈ +0.8% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 5986 ms (33 iters) | ✅ 5962 ms (33 iters) | ≈ -0.4% (±25%) | |
| finagle-chirper | 25 | ✅ 5436 ms (36 iters) | ✅ 5463 ms (36 iters) | ≈ +0.5% (±24.4%) | |
| fj-kmeans | 21 | ✅ 2773 ms (67 iters) | ✅ 2635 ms (72 iters) | 🟢 -5% | — / — |
| fj-kmeans | 25 | ✅ 2765 ms (68 iters) | ✅ 2836 ms (66 iters) | ≈ +2.6% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2053 ms (90 iters) | ✅ 2073 ms (89 iters) | ≈ +1% (±2.6%) | — / — |
| future-genetic | 25 | ✅ 1989 ms (93 iters) | ✅ 2089 ms (89 iters) | 🔴 +5% | — / — |
| naive-bayes | 21 | ✅ 1249 ms (137 iters) | ✅ 1262 ms (136 iters) | ≈ +1% (±33%) | — / — |
| naive-bayes | 25 | ✅ 1015 ms (168 iters) | ✅ 1005 ms (170 iters) | ≈ -1% (±31.5%) | — / — |
| reactors | 21 | ✅ 15838 ms (16 iters) | ✅ 16414 ms (15 iters) | ≈ +3.6% (±7.8%) | — / — |
| reactors | 25 | ✅ 18212 ms (15 iters) | ✅ 18465 ms (15 iters) | ≈ +1.4% (±5.1%) | — / — |
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 / 5 | 1966 / 1969 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2264 / 2245 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 8633 / 8777 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 8137 / 8242 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1282 / 1291 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 5 | 1260 / 1274 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 2919 / 2987 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2845 / 2956 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 3505 / 3511 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3477 / 3516 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1782 / 1516 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1933 / 1963 | ✅ / ✅ | ✅ / ✅ |
…r into zgu/fault-injection
zhengyu123
marked this pull request as ready for review
July 17, 2026 13:13
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af63690a85
ℹ️ 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".
Contributor
Benchmark Results (commit af63690)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125280286 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10213 ms (21 iters) | ✅ 10338 ms (21 iters) | ≈ +1.2% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 8848 ms (24 iters) | ✅ 8958 ms (24 iters) | ≈ +1.2% (±10.8%) | — / — |
| finagle-chirper | 21 | ✅ 5972 ms (33 iters) | ✅ 5977 ms (33 iters) | ≈ +0.1% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5475 ms (36 iters) | ✅ 5445 ms (36 iters) | ≈ -0.5% (±24.9%) | |
| fj-kmeans | 21 | ✅ 2703 ms (70 iters) | ✅ 2710 ms (69 iters) | ≈ +0.3% (±2.7%) | — / — |
| fj-kmeans | 25 | ✅ 2763 ms (68 iters) | ✅ 2801 ms (66 iters) | ≈ +1.4% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2089 ms (89 iters) | ✅ 2085 ms (88 iters) | ≈ -0.2% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2088 ms (90 iters) | ✅ 1964 ms (94 iters) | 🟢 -5.9% | — / — |
| naive-bayes | 21 | ✅ 1251 ms (137 iters) | ✅ 1246 ms (137 iters) | ≈ -0.4% (±32.5%) | — / — |
| naive-bayes | 25 | ✅ 1020 ms (168 iters) | ✅ 1007 ms (170 iters) | ≈ -1.3% (±31.2%) | — / — |
| reactors | 21 | ✅ 16024 ms (15 iters) | ✅ 16871 ms (15 iters) | ≈ +5.3% (±7.1%) | — / — |
| reactors | 25 | ✅ 18316 ms (15 iters) | ✅ 18666 ms (15 iters) | ≈ +1.9% (±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 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 1950 / 2021 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2375 / 2286 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 8783 / 8853 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 8204 / 8510 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 1 | 1258 / 1277 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1298 / 1267 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2929 / 2933 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 2931 / 2820 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 3490 / 3498 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 4 | 3492 / 3477 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1629 / 1690 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1832 / 1950 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c853923927
ℹ️ 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".
| assert(crashProtectionActive() || SafeAccess::isReadable(ptr)); | ||
| return ptr; | ||
| // Poison only the returned pointer; the assert above sees the real ptr. | ||
| return INJECT_FAULT_ADDRESS_RARE(ptr); |
There was a problem hiding this comment.
When -PenableFaultInjection is used, this poisons every VMStructs::at() result, but many callers raw-dereference it outside both SafeAccess and walkVM's setjmp protection—for example VMFlag::find()/VMFlag::set() during VM initialization and JVMAccess flag reads. If the rare injection fires in any of those paths, the SIGSEGV is not recovered (some occur before profiler crash protection is active), so a fault-injected build can randomly crash on unrelated VMStructs access instead of exercising the intended recovery paths. Please restrict this to call sites that are actually protected or wrap the individual dereferences with the appropriate recovery mechanism.
Useful? React with 👍 / 👎.
| if (sig == SIGSEGV || sig == SIGBUS) { | ||
| if (pc == (uintptr_t)safefetch32_impl) { | ||
| uc->current_pc = (uintptr_t)safefetch32_cont; | ||
| Counters::increment(SAFEFETCH_FAILED); |
There was a problem hiding this comment.
This runs inside the SIGSEGV/SIGBUS handler path (Profiler::crashHandlerInternal calls SafeAccess::handle_safefetch() before any reentrancy guard), and Counters::increment() can lazily initialize the counter array via Counters::init() (aligned_alloc/memset) on first use. If the first safefetch failure happens before counters were reset/queried, or while the fault interrupted allocator code, the signal handler can allocate or take allocator locks while recovering a safefetch fault. Please pre-initialize these counters off the signal path or avoid counting from this handler.
Useful? React with 👍 / 👎.
Contributor
Benchmark Results (commit c853923)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/125329265 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10331 ms (21 iters) | ✅ 10160 ms (21 iters) | ≈ -1.7% (±10.7%) | — / — |
| akka-uct | 25 | ✅ 8818 ms (24 iters) | ✅ 8934 ms (24 iters) | ≈ +1.3% (±10.2%) | — / — |
| finagle-chirper | 21 | ✅ 5953 ms (33 iters) | ✅ 5983 ms (33 iters) | ≈ +0.5% (±25.6%) | |
| finagle-chirper | 25 | ✅ 5502 ms (36 iters) | ✅ 5530 ms (36 iters) | ≈ +0.5% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2715 ms (69 iters) | ✅ 2831 ms (66 iters) | 🔴 +4.3% | — / — |
| fj-kmeans | 25 | ✅ 2821 ms (66 iters) | ✅ 2814 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2107 ms (88 iters) | ✅ 2169 ms (86 iters) | 🔴 +2.9% | — / — |
| future-genetic | 25 | ✅ 2068 ms (90 iters) | ✅ 2065 ms (90 iters) | ≈ -0.1% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1288 ms (133 iters) | ✅ 1311 ms (130 iters) | ≈ +1.8% (±33.2%) | — / — |
| naive-bayes | 25 | ✅ 1020 ms (168 iters) | ✅ 995 ms (172 iters) | ≈ -2.5% (±31.1%) | — / — |
| reactors | 21 | ✅ 16279 ms (15 iters) | ✅ 16147 ms (15 iters) | ≈ -0.8% (±8.5%) | — / — |
| reactors | 25 | ✅ 18636 ms (15 iters) | ✅ 18161 ms (15 iters) | ≈ -2.5% (±5.6%) | — / — |
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 / 2 | 2080 / 1959 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2299 / 2174 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 8613 / 8380 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 8514 / 8095 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 1241 / 1276 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1286 / 1278 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3005 / 3058 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 2920 / 2944 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 8 / 2 | 3510 / 3522 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 3441 / 3462 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 2 | 1638 / 1821 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1829 / 1842 | ✅ / ✅ | ✅ / ✅ |
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
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?:
Adds a compile-time fault-injection layer that deliberately corrupts pointers at the profiler's real memory-access sites so the existing recovery paths (SafeAccess safefetch and walkVM's setjmp/longjmp) are exercised continuously under test. Entirely gated behind FAULT_INJECTION: when the flag is absent every construct is a strict identity/no-op with zero runtime cost, so production release/debug/asan/tsan builds are unaffected.
To build fault-injected binaries, add
-PenableFaultInjectionflag to the command line, e.g.This iteration emphasizes on installing the framework, with limited fault injection sites. More injection sites will be added in followup PRs.
Motivation:
Motivation and goal is documented here
Additional Notes:
What's included
faultInjection.h — three probability tiers as integer xorshift thresholds (RARE 0.01%, UNLIKELY 0.1%, LIKELY 1%) and the INJECT_FAULT_{ADDRESS,INT,LONG}_{RARE,UNLIKELY,LIKELY} macros. When the flag is off, each macro is a strict identity ((ptr)/(v)).
faultInjection.cpp — PRNG, PROT_NONE guard-region setup, and poisonAddress() (mix of a mmap'd guard-page pointer and random non-canonical addresses).
Per-thread PRNG
threadLocalData.h — per-thread xorshift64 state in ProfiledThread (async-signal-safe), with a global atomic fallback.
Injection sites (wrap the dereferenced pointer)
stackWalker.cpp — walkFP / walkDwarf SafeAccess loads (LIKELY) → safefetch recovery.
hotspot/hotspotSupport.cpp — walkVM raw derefs (UNLIKELY) → setjmp/longjmp recovery.
hotspot/vmStructs.h — VMStructs::at() (RARE).
profiler.cpp — calls faultinj::init() at signal-handler setup.
Counters (counters.h, live under -DCOUNTERS)
safecopy_failed, safefetch_failed, faults_injected, walkvm_longjmp_recovered — instrument the recovery mechanisms so injection activity and each recovery route are observable.
Build wiring
-PenableFaultInjectionappends-D__FAULT_INJECTION__to the standard release/debug library builds (so buildDebug/buildRelease produce a fault-injected libjavaProfiler.so) and to the gtest build. Deliberately not applied to asan/tsan/fuzzer, which install their own SIGSEGV interception.faultInjection_ut.cpp — disabled-path identity checks (default build) plus enabled-path tier-rate bands and SafeAccess/walkVM recovery tests (-PenableFaultInjection).
How to test the change?:
./gradlew :ddprof-lib:gtestDebug_faultInjection_ut -PenableFaultInjection— 6 enabled cases pass../gradlew :ddprof-lib:gtestDebug_faultInjection_ut (default)— 2 identity cases pass../gradlew -PenableFaultInjection :ddprof-lib:assembleDebug— verified faultinj::* symbols present (.symtab; hidden visibility, not exported).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!