Adopt ThreadLocal to store ProfiledThread#639
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates ProfiledThread storage from a raw pthread_key_t TLS implementation to the project’s ThreadLocal<> wrapper, and wires up an explicit native initialization path so per-thread profiler metadata can be established earlier and more predictably.
Changes:
- Replace
ProfiledThread’spthread_key_tTLS plumbing withThreadLocal<ProfiledThread*>and adjust initialization APIs accordingly. - Add
Profiler::init()and call it fromJavaProfiler_init0()to initialize native state earlier. - Update JVM support initialization to also validate the
ProfiledThreadTLS key.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ddprof-lib/src/test/cpp/ddprof_ut.cpp | Updates unit test to use the new initCurrentThread() return value. |
| ddprof-lib/src/main/cpp/threadLocalData.h | Switches ProfiledThread to ThreadLocal<> and adjusts initialization/lookup API surface. |
| ddprof-lib/src/main/cpp/threadLocalData.cpp | Implements the new ThreadLocal<>-backed ProfiledThread initialization and teardown logic. |
| ddprof-lib/src/main/cpp/profiler.h | Adds Profiler::init() to the public interface. |
| ddprof-lib/src/main/cpp/profiler.cpp | Implements Profiler::init() and uses initCurrentThread() where needed. |
| ddprof-lib/src/main/cpp/jvmSupport.cpp | Treats ProfiledThread TLS key validity as part of JVM support initialization readiness. |
| ddprof-lib/src/main/cpp/javaApi.cpp | Calls Profiler::init() from JNI init and updates several JNI entry points to use the new thread initialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
CI Test ResultsRun: #29118545378 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Testsglibc-aarch64/debug / 8-j9Job: View logs No detailed failure information available. Check the job logs. Summary: Total: 32 | Passed: 31 | Failed: 1 Updated: 2026-07-10 19:54:39 UTC |
Reliability & Chaos Results✅ All reliability & chaos checks passed Pipeline: https://gitlab.ddbuild.io/DataDog/java-profiler/-/pipelines/124097733 |
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>
…r into zgu/profiled_thread
![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: 9ded4c2554
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d1088337d
ℹ️ 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".
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
I have some comments where changes or clarification might be necessary
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d59a7c2f1f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00edbd4548
ℹ️ 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 00edbd4)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124031547 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 25 | ✅ 8913 ms (24 iters) | ✅ 8909 ms (24 iters) | ≈ -0% (±10.2%) | — / — |
| finagle-chirper | 21 | ✅ 5983 ms (33 iters) | ✅ 5960 ms (33 iters) | ≈ -0.4% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5520 ms (36 iters) | ✅ 5473 ms (36 iters) | ≈ -0.9% (±25%) | |
| fj-kmeans | 21 | ✅ 2713 ms (69 iters) | ✅ 2833 ms (66 iters) | 🔴 +4.4% | — / — |
| fj-kmeans | 25 | ✅ 2767 ms (68 iters) | ✅ 2841 ms (66 iters) | ≈ +2.7% (±2.7%) | — / — |
| future-genetic | 21 | ✅ 2125 ms (87 iters) | ✅ 2038 ms (91 iters) | 🟢 -4.1% | — / — |
| future-genetic | 25 | ✅ 2049 ms (91 iters) | ✅ 2107 ms (88 iters) | 🔴 +2.8% | — / — |
| naive-bayes | 21 | ✅ 1266 ms (136 iters) | ✅ 1227 ms (139 iters) | ≈ -3.1% (±32.4%) | — / — |
| naive-bayes | 25 | ✅ 1015 ms (169 iters) | ✅ 1019 ms (168 iters) | ≈ +0.4% (±31.7%) | — / — |
| reactors | 21 | ✅ 16763 ms (15 iters) | ✅ 16497 ms (15 iters) | ≈ -1.6% (±6.5%) | — / — |
| reactors | 25 | ✅ 18424 ms (15 iters) | ✅ 18754 ms (15 iters) | ≈ +1.8% (±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 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 2227 / 2257 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 8486 / 8474 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 8548 / 8263 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1291 / 1261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1260 / 1275 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 3020 / 2985 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2905 / 2855 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 9 / 7 | 3539 / 3501 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 3451 / 3524 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1796 / 1600 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1881 / 1900 | ✅ / ✅ | ✅ / ✅ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 800d38db45
ℹ️ 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 800d38d)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124059019 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):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8f9c02d3b
ℹ️ 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 d8f9c02)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124089746 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10431 ms (21 iters) | ✅ 10308 ms (21 iters) | ≈ -1.2% (±11.3%) | — / — |
| akka-uct | 25 | ✅ 8866 ms (24 iters) | ✅ 8827 ms (24 iters) | ≈ -0.4% (±10.1%) | — / — |
| finagle-chirper | 21 | ✅ 6098 ms (33 iters) | ✅ 5997 ms (33 iters) | ≈ -1.7% (±25.7%) | |
| finagle-chirper | 25 | ✅ 5497 ms (36 iters) | ✅ 5446 ms (36 iters) | ≈ -0.9% (±24.9%) | |
| fj-kmeans | 21 | ✅ 2723 ms (68 iters) | ✅ 2777 ms (67 iters) | ≈ +2% (±2.8%) | — / — |
| fj-kmeans | 25 | ✅ 2835 ms (66 iters) | ✅ 2839 ms (66 iters) | ≈ +0.1% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2141 ms (87 iters) | ✅ 2057 ms (90 iters) | 🟢 -3.9% | — / — |
| future-genetic | 25 | ✅ 1985 ms (94 iters) | ✅ 2135 ms (87 iters) | 🔴 +7.6% | — / — |
| naive-bayes | 21 | ✅ 1302 ms (132 iters) | ✅ 1256 ms (136 iters) | ≈ -3.5% (±31.9%) | — / — |
| naive-bayes | 25 | ✅ 1027 ms (166 iters) | ✅ 1010 ms (170 iters) | ≈ -1.7% (±31.4%) | — / — |
| reactors | 21 | ✅ 16354 ms (15 iters) | ✅ 17370 ms (15 iters) | ≈ +6.2% (±7.5%) | — / — |
| reactors | 25 | ✅ 18244 ms (15 iters) | ✅ 18339 ms (15 iters) | ≈ +0.5% (±3.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 / 4 | 2011 / 1965 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 2391 / 2319 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 9018 / 8712 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 8134 / 8330 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 3 | 1270 / 1259 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1300 / 1276 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 3048 / 2908 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 2884 / 2912 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 3507 / 3486 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 3 | 3463 / 3465 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 1583 / 1875 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1865 / 1860 | ✅ / ✅ | ✅ / ✅ |
Benchmark Results (commit 42fd898)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124097745 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10279 ms (21 iters) | ✅ 10420 ms (21 iters) | ≈ +1.4% (±11.9%) | — / — |
| akka-uct | 25 | ✅ 8908 ms (24 iters) | ✅ 8891 ms (24 iters) | ≈ -0.2% (±10.4%) | — / — |
| finagle-chirper | 21 | ✅ 6014 ms (33 iters) | ✅ 5963 ms (33 iters) | ≈ -0.8% (±25.3%) | |
| finagle-chirper | 25 | ✅ 5394 ms (36 iters) | ✅ 5439 ms (36 iters) | ≈ +0.8% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2808 ms (66 iters) | ✅ 2752 ms (68 iters) | ≈ -2% (±2.6%) | — / — |
| fj-kmeans | 25 | ✅ 2845 ms (66 iters) | ✅ 2832 ms (66 iters) | ≈ -0.5% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2090 ms (89 iters) | ✅ 2098 ms (88 iters) | ≈ +0.4% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2081 ms (89 iters) | ✅ 2068 ms (90 iters) | ≈ -0.6% (±2.6%) | — / — |
| naive-bayes | 21 | ✅ 1237 ms (138 iters) | ✅ 1269 ms (135 iters) | ≈ +2.6% (±32.9%) | — / — |
| naive-bayes | 25 | ✅ 1017 ms (168 iters) | ✅ 994 ms (172 iters) | ≈ -2.3% (±31.4%) | — / — |
| reactors | 21 | ✅ 16013 ms (15 iters) | ✅ 17296 ms (15 iters) | 🔴 +8% | — / — |
| reactors | 25 | ✅ 18492 ms (15 iters) | ✅ 17989 ms (15 iters) | ≈ -2.7% (±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 / 2 | 1956 / 2034 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 2260 / 2253 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 3 | 8448 / 8480 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 7938 / 8112 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1277 / 1292 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 2 | 1293 / 1285 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 4 / ✅ | 3042 / 2968 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2921 / 2874 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 6 | 3483 / 3496 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 4 / 3 | 3476 / 3471 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1793 / 1832 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1820 / 1908 | ✅ / ✅ | ✅ / ✅ |
What does this PR do?:
Replaces ProfiledThread's hand-rolled pthread_key_t + pthread_once/atomic-flag TLS management with the existing
ThreadLocal<T, CREATE_FUNC, CLEAN_FUNC>template, and closes two related gaps that fell out of that consolidation:ProfiledThread::current()is now the single signal-safe accessor (never allocates, returns existing TLS or nullptr), replacing the old split betweencurrent()(lazy-allocating) andcurrentSignalSafe()(non-allocating).currentSignalSafe()is removed; all 39 call sites across 16 files were renamed tocurrent().initCurrentThread() / initCurrentThreadSignalSafe()are the allocating variants and now return theProfiledThread*(ornullptr) instead ofvoid, so callers can check the result instead of asserting.ProfiledThread::isThreadKeyValid()is a new, cheap validity check backed by the ThreadLocal's own key state.JVMSupport::initialize() / isInitialized()now checkisThreadKeyValid()for real, closing a previously-stubbed gap ("Add ProfiledThread key checking here in next PR").Profiler::init()is new — called fromJavaProfiler_init0beforeVM::initProfilerBridge()— and runscheckState()under_state_lockbefore callinginitCurrentThreadSignalSafe(), closing a startup race where native TLS init could otherwise run ahead of a failed profiler state check.JNI hot paths in javaApi.cpp (
filterThreadAdd0/filterThreadRemove0) now return early on a nullcurrent()instead of relying on a debug-only assert; a newinitOrGetCurrentThread()helper covers the early-startup TLS race ininitializeContextTLS0.Native-thread wrappers in libraryPatcher_linux.cpp were updated to consume the new
initCurrentThread()return value with explicit null checks.Motivation:
The old TLS implementation duplicated logic that ThreadLocal already provides (key creation, lazy allocation, signal-safe clear-then-free ordering), and left two known gaps: JVMSupport never actually validated the ProfiledThread key, and native-thread/JNI call sites relied on assert (a no-op in release builds) instead of real null checks — both closed here.
Additional Notes:
How to test the change?:
Existing gtest suites (thread_teardown_safety_ut.cpp, ddprof_ut.cpp, hotspot_crash_protection_ut.cpp, jvmSupport_ut.cpp) were updated for the renamed/reshaped API, plus a new regression test, JvmSupportInitFailureTest.InitReturnsErrorAndLatchesErrorState, covering Profiler::init()'s error path when JVMSupport::initialize() fails.
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!