Use ThreadLocal to setup protection longjmp buffer and fix jmp_buf chaining#615
Conversation
|
CI Test ResultsRun: #28827871146 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-06 22:52:10 UTC |
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>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Reliability & Chaos Results❌ 2 failure(s) detected Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/123082923 ❌ chaos: profiler tcmalloc amd64 21 0 3 temXchaos❌ chaos: profiler tracer tcmalloc aarch64 25 0 3 temXchaos |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ofiler into zgu/objmonitor_deflation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Benchmark Results (commit e4b9719)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/122633659 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10389 ms (7 iters) | ✅ 10329 ms (7 iters) | ≈ -0.6% (±20.8%) | — / — |
| akka-uct | 25 | ✅ 8966 ms (8 iters) | ✅ 8809 ms (8 iters) | ≈ -1.8% (±20.3%) | — / — |
| finagle-chirper | 21 | ✅ 6023 ms (11 iters) | ✅ 6002 ms (11 iters) | ≈ -0.3% (±44.9%) | |
| finagle-chirper | 25 | ✅ 5457 ms (12 iters) | ✅ 5438 ms (12 iters) | ≈ -0.3% (±43.4%) | |
| fj-kmeans | 21 | ✅ 2875 ms (22 iters) | ✅ 2730 ms (23 iters) | 🟢 -5% | — / — |
| fj-kmeans | 25 | ✅ 2633 ms (24 iters) | ✅ 2842 ms (22 iters) | 🔴 +7.9% | — / — |
| future-genetic | 21 | ✅ 2133 ms (29 iters) | ✅ 2080 ms (30 iters) | ≈ -2.5% (±4.2%) | — / — |
| future-genetic | 25 | ✅ 2070 ms (30 iters) | ✅ 2141 ms (29 iters) | ≈ +3.4% (±4.3%) | — / — |
| naive-bayes | 21 | ✅ 1221 ms (46 iters) | ✅ 1277 ms (45 iters) | ≈ +4.6% (±57.5%) | — / — |
| naive-bayes | 25 | ✅ 1034 ms (55 iters) | ✅ 1018 ms (56 iters) | ≈ -1.5% (±55.8%) | — / — |
| reactors | 21 | ✅ 16041 ms (5 iters) | ✅ 15714 ms (5 iters) | ≈ -2% (±17.4%) | — / — |
| reactors | 25 | ✅ 17707 ms (5 iters) | ✅ 17823 ms (5 iters) | ≈ +0.7% (±11.5%) | — / — |
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 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 4 | 2466 / 2284 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8316 / 8172 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 4 | 1298 / 1271 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2914 / 2977 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 1 | 3478 / 3494 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / 1597 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1765 / 1822 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d3ad2be2
ℹ️ 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 4b571c4)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/122986649 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10142 ms (7 iters) | ✅ 10374 ms (7 iters) | ≈ +2.3% (±21.9%) | — / — |
| akka-uct | 25 | ✅ 8697 ms (8 iters) | ✅ 8704 ms (8 iters) | ≈ +0.1% (±16.9%) | — / — |
| finagle-chirper | 21 | ✅ 6027 ms (11 iters) | ✅ 5956 ms (11 iters) | ≈ -1.2% (±45.3%) | |
| finagle-chirper | 25 | ✅ 5449 ms (12 iters) | ✅ 5449 ms (12 iters) | ≈ 0% (±43.5%) | |
| fj-kmeans | 21 | ✅ 2872 ms (22 iters) | ✅ 2727 ms (23 iters) | 🟢 -5% | — / — |
| fj-kmeans | 25 | ✅ 2824 ms (22 iters) | ✅ 2791 ms (22 iters) | ≈ -1.2% (±4.6%) | — / — |
| future-genetic | 21 | ✅ 2034 ms (30 iters) | ✅ 2088 ms (30 iters) | ≈ +2.7% (±4.3%) | — / — |
| future-genetic | 25 | ✅ 2018 ms (31 iters) | ✅ 2131 ms (29 iters) | 🔴 +5.6% | — / — |
| naive-bayes | 21 | ✅ 1234 ms (46 iters) | ✅ 1327 ms (43 iters) | ≈ +7.5% (±58.8%) | — / — |
| naive-bayes | 25 | ✅ 1017 ms (56 iters) | ✅ 1017 ms (56 iters) | ≈ 0% (±55.2%) | — / — |
| reactors | 21 | ✅ 16572 ms (5 iters) | ✅ 18390 ms (5 iters) | 🔴 +11% | — / — |
| reactors | 25 | ✅ 18269 ms (5 iters) | ✅ 18396 ms (5 iters) | ≈ +0.7% (±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 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2289 / 2053 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 8650 / ✅ | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 8159 / 8478 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 5 | ✅ / 1268 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 7 / 4 | 1275 / 1265 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2953 / ✅ | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2941 / ✅ | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 5 / ✅ | 3490 / ✅ | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 3489 / 3508 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1692 / 1976 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1875 / 1835 | ✅ / ✅ | ✅ / ✅ |
rkennke
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you!
@rkennke Thank you for reviewing. |
jbachorik
left a comment
There was a problem hiding this comment.
A few minor/clarifying comments. Otherwise good to go.
…from deleted thread.h to threadLocalData.h PR #615 (origin/main) renamed thread.cpp to threadLocalData.cpp and deleted thread.h, absorbing ProfiledThread into threadLocalData.h. Our branch had modified thread.h to add per-thread JFR-inflight tracking. Resolution: - Delete thread.h (accept deletion from main) - Port _registry_head/_registry_lock static members to threadLocalData.h - Port _jfr_inflight/_registry_next instance members to threadLocalData.h - Port enterJfrInflight/exitJfrInflight/jfrInflight methods to threadLocalData.h - Port forEachRegistered/registryInsert/registryRemove declarations to threadLocalData.h - Update signalInflight.cpp to include threadLocalData.h instead of thread.h - threadLocalData.cpp already has implementation (correctly auto-merged)
What does this PR do?:
This PR intents to resolve some of mysterious crashes seen in later releases, e.g. PROF-15206, although, it might not actually solve this particular problem, but I believe it should have positive impact - Cloude and I believe that overwriting JavaThread's exception field is dangerous, in fact, overwriting any JVM data structures are dangerous, because the writes can result in races and readers may see wrong values, that can lead to crashes.
Before introducing Thread Local Handshake in JVM, thread object is usually only read by 'current` thread, so it is relative safe to alter, then restore values inside signal handler, but not any more - a thread may be able to read other thread's states before it decides how to proceed the handshake, so it may read wrong values.
This PR:
Avoid altering
JavaThreadfor jmp_bufUses a thread local for storing longjmp buffer and eliminates the dependency on overwriting
JavaThread'sexceptionfield, solongjmpnow protects all threads, not justJavaThread- should improve stability.Chaining jmp_buf
A none signal based sampler can be interrupted by signal based sampler. Therefore, there may be multiple
walkVM()calls on stack, each of them sets up a protectionjmp_buffor stack walk, thejmp_bufs need to be chained to ensure to jump back to expected location.Motivation:
Improve profiler's stability.
Additional Notes:
In additional,
exceptionfield only exists inJavaThread, but detection code in Profiler is somewhat questionable, especially, for the subclasses ofJavaThreadHow to test the change?:
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!