Store per_thread_context in each Ruby Thread by eregon · Pull Request #5816 · DataDog/dd-trace-rb

eregon · 2026-05-27T13:20:36Z

What does this PR do?

Store the per_thread_context in the Ruby Thread TLS (rb_internal_thread_specific_set()) so all the state is in one struct and not a struct + a single field in the TLS. Also we'll need more place in TLS for #5777 (comment) but there is a hard limit on TLS, so this avoids that problem.

Motivation:

Much simpler and more consistent, the state is together vs spread in different places.

Change log entry

Yes. Remove overhead related to cleaning up profiler state for dead threads.

Additional Notes:

How to test the change?

datadog-official · 2026-05-27T13:22:19Z

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 98.04%
• Overall Coverage: 97.07% (+0.01%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: 91ccef0 | Docs | Datadog PR Page | Give us feedback!}

pr-commenter · 2026-05-27T14:51:32Z

Benchmarks

Benchmark execution time: 2026-06-04 12:12:20

Comparing candidate commit 91ccef0 in PR branch store-per_thread_context-in-Ruby-Thread with baseline commit 8593e2e in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 44 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

🟩 = significantly better candidate vs. baseline
🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:profiling - Allocations ()

🟥 throughput [-282701.775op/s; -242061.359op/s] or [-8.593%; -7.357%]

eregon · 2026-05-27T15:46:58Z

Mmh, 3.2 doesn't seem to like rb_thread_current() being called from on_gvl_event():
https://github.com/DataDog/dd-trace-rb/actions/runs/26517141533/job/78097642763?pr=5816#step:5:3517

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 913bb24b83

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

dd-octo-sts · 2026-05-28T13:47:37Z

Typing analysis

This PR does not change typing compared to the base branch.

eregon · 2026-05-29T09:46:27Z

Another idea design-wise given that both ThreadContext and CpuAndWallTimeWorker are de-facto global would be to have a single instance of them, always and be able to reinitialize/reconfigure them.
Then at least we wouldn't need to worry about things like multiple st_tables of threads mapping to a single global TLS variable per thread.
OTOH it would be true global state and example, the previous pre_thread_context would be remembered unless we clear them explicitly during reinitialize.
If we clear them it probably wouldn't change much compared to the current approach of stopping the previous instance before replacing it with a new one.
TLDR: seems worse than the current approach but wanted to mention it in case it would turn into something useful.

ivoanjo

I haven't yet finished doing a full pass but need to switch gears! Here's what I got so far...

ivoanjo

👍 Really nice! Thanks for addressing all my notes and nitpicks 🙏

…=5 bundle exec rspec ...` * Very useful to repro transient tests. Re-running the full rspec command multiple times is much slower and less likely to find the transient.

eregon · 2026-06-04T08:42:54Z

Benchmarks

scenario:profiling - stack collector (varying depth)

🟩 throughput [+5569.946op/s; +5613.529op/s] or [+574.215%; +578.708%]

Nice, and I can repro locally, 3.417k -> 23.331k.
I think this is mainly because we reuse the locations buffer, which is now always stored in the ThreadContext and before was allocated+free on every _native_sample() call.

eregon · 2026-06-04T11:31:24Z

This seems a pre-existing transient but I fixed it anyway. If the thread was already dead when we sample then we won't init any state for it, AFAIK even before this PR.

* Store the per_thread_context in the Ruby Thread TLS (rb_internal_thread_specific_set()), so all the state is in one struct and not a struct + a single field in the TLS. * The TLS can be read safely in async-signal safe functions and functions without the GVL. * The per_thread_context for a given Thread lives for the whole Thread lifetime, that way there is no possible mistmatch with a new instance of Datadog::Profiling::Collectors::ThreadContext. * We'll need more place in TLS for #5777 (comment) but there is a hard limit on the number of TLS vars, so this avoids that problem. * Much simpler and more consistent, the state is together vs spread in different places. * Remove hash_map_per_thread_context and its dead theads cleanups, they are now redundant. * Use a TypedData object stored in a hidden instance variable on the Thread for marking the sampling_buffer. Skip profiling frozen threads to avoid FrozenError when setting the instance variable. * Move locations out of sampling_buffer into collector state, since anyway its lifetime was tied to the collector and not the buffers. * Do not call get_or_create_context_for() in thread_context_collector_sample_allocation(), because get_or_create_context_for() allocates. * Clear stale per_thread_context between tests to fix transient test failures and make testing easier. The TLS-based per_thread_context persists across collector instances, so stale gc_tracking state from a previous test caused all cpu-time samples to read as 0. Co-Authored-By: Claude Opus 4.6 <[email protected]>

…s and fix specs * In a real fork() we should only clean the current thread, so do that and fix the specs. * Also remove duplication.

…extremely rare

…ions size Since per_thread_context persists in Thread TLS across collector instances, a new collector with a smaller max_frames would write past its locations array using the old buffer's max_frames. Fix: pass sample_locations (ptr + len) to sample_thread, cap captured_frames to locations.len, and reinitialize the buffer afterward to reconcile sizes. Co-Authored-By: Claude Opus 4.6 <[email protected]>

The _native_new_empty_thread helper's thread body returned immediately, so the thread could die before sample ran. Since per_thread_context is now only built for live threads, fetch raised KeyError when the thread was already dead. Co-Authored-By: Claude Opus 4.6 <[email protected]>

dd-octo-sts Bot added the profiling Involves Datadog profiling label May 27, 2026

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch 2 times, most recently from a9f446a to d430c5d Compare May 27, 2026 14:21

eregon commented May 27, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch 3 times, most recently from d0236e0 to 913bb24 Compare May 28, 2026 12:37

eregon marked this pull request as ready for review May 28, 2026 12:37

eregon requested review from a team as code owners May 28, 2026 12:37

eregon requested a review from ivoanjo May 28, 2026 12:37

chatgpt-codex-connector Bot reviewed May 28, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch from 913bb24 to 258c15c Compare May 28, 2026 13:47

eregon commented May 29, 2026

View reviewed changes

Comment thread lib/datadog/profiling/collectors/thread_context.rb Outdated

ivoanjo reviewed May 29, 2026

View reviewed changes

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch 5 times, most recently from fd5ebfd to 27864bf Compare June 1, 2026 21:12

eregon commented Jun 2, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_stack.h

eregon commented Jun 2, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c

eregon commented Jun 2, 2026

View reviewed changes

Comment thread spec/datadog/profiling/collectors/thread_context_spec.rb

ivoanjo reviewed Jun 2, 2026

View reviewed changes

eregon commented Jun 2, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch from 77e8026 to 5e775e8 Compare June 3, 2026 13:27

ivoanjo approved these changes Jun 3, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/datadog_ruby_common.c

eregon added 2 commits June 4, 2026 10:30

Fix alignment in Waiting for GVL comment and clarify

bc525b8

Add a way to run each RSpec example multiple times with `RSPEC_REPEAT…

614f83f

…=5 bundle exec rspec ...` * Very useful to repro transient tests. Re-running the full rspec command multiple times is much slower and less likely to find the transient.

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch from 5e775e8 to 98cd669 Compare June 4, 2026 08:31

eregon enabled auto-merge June 4, 2026 08:31

eregon disabled auto-merge June 4, 2026 08:38

ivoanjo reviewed Jun 4, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch 2 times, most recently from d8490cd to f360872 Compare June 4, 2026 10:57

ivoanjo reviewed Jun 4, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_stack.c

Comment thread ext/datadog_profiling_native_extension/collectors_stack.c

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon enabled auto-merge June 4, 2026 11:39

eregon disabled auto-merge June 4, 2026 11:39

eregon and others added 6 commits June 4, 2026 13:45

Add note that reset_after_fork is called in the same process for spec…

1212044

…s and fix specs * In a real fork() we should only clean the current thread, so do that and fix the specs. * Also remove duplication.

Raise if a Thread is frozen as we cannot profile it and it should be …

8c97b7b

…extremely rare

Sample explicitly in allocation spec

1e48abd

eregon force-pushed the store-per_thread_context-in-Ruby-Thread branch from 4e5d5df to 91ccef0 Compare June 4, 2026 11:46

eregon enabled auto-merge June 4, 2026 12:02

eregon disabled auto-merge June 4, 2026 12:02

eregon enabled auto-merge June 4, 2026 12:02

eregon merged commit 5ca32b1 into master Jun 4, 2026
592 checks passed

eregon deleted the store-per_thread_context-in-Ruby-Thread branch June 4, 2026 12:14

dd-octo-sts Bot added this to the 2.36.0 milestone Jun 4, 2026

This was referenced Jun 22, 2026

[NO-TICKET] Profiling: Reset leftover per-thread state when creating new ThreadContext #5926

Closed

[NO-TICKET] Profiling: Reset per-thread state when starting profiler #5960

Merged

Uh oh!

Conversation

eregon commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

datadog-official Bot commented May 27, 2026 • edited by datadog-prod-us1-3 Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pr-commenter Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Explanation

More details about the CI and significant changes

scenario:profiling - Allocations ()

Uh oh!

eregon commented May 27, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

dd-octo-sts Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Typing analysis

Uh oh!

Uh oh!

eregon commented May 29, 2026

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eregon commented Jun 4, 2026

Benchmarks

scenario:profiling - stack collector (varying depth)

Uh oh!

Uh oh!

eregon commented Jun 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eregon commented May 27, 2026 •

edited

Loading

datadog-official Bot commented May 27, 2026 •

edited by datadog-prod-us1-3 Bot

Loading

pr-commenter Bot commented May 27, 2026 •

edited

Loading

dd-octo-sts Bot commented May 28, 2026 •

edited

Loading