Skip sampling threads which did not have the GVL since the last sample by eregon · Pull Request #5777 · DataDog/dd-trace-rb

eregon · 2026-05-19T11:09:30Z

Since such threads did not get the GVL then their Ruby-level stack must
be the same and there is no need to sample them, we can just adjust the
duration/weight of the next sample instead.
The next sample will have the correct stack because it will be just after
GVL acquire.
Emit a catch-up sample for still-suspended threads before serialization
so we still sample at least once per upload period (60 seconds).
The per-Ruby-Thread state is an integer:
- low bit: current state (1 = suspended/no-GVL, 0 = running/has-GVL)
- bits 1+: monotonic event counter, incremented on every SUSPENDED or RESUMED for that Thread

What does this PR do?

Reduce profiling overhead by skipping redundant samples of threads which did not have the GVL since the last sample.

Motivation:
Reduce profiling overhead.

Change log entry

Reduce profiling overhead for threads which do not hold the GVL.

Additional Notes:

How to test the change?

dd-octo-sts · 2026-05-19T11:09:47Z

👋 Hey @eregon, please fill "Change log entry" section in the pull request description.

If changes need to be present in CHANGELOG.md you can state it this way

**Change log entry**

Yes. A brief summary to be placed into the CHANGELOG.md

(possible answers Yes/Yep/Yeah)

Or you can opt out like that

**Change log entry**

None.

(possible answers No/Nope/None)

^{Visited at: 2026-05-19 11:09:47 UTC}

datadog-prod-us1-3 · 2026-05-19T11:12:30Z

Tests

✨ Fix all issues with BitsAI or with Cursor

⚠️ Warnings

❄️ 2 New flaky tests detected

Datadog::Profiling::Collectors::ThreadContext SUSPENDED-skip optimization when a thread is suspended (does not hold the GVL) skips per-tick samples for the suspended thread from rspec (Fix with Cursor)
expected: 0
     got: 2

(compared using ==)

Failure/Error: expect(stats.fetch(:inactive_thread_samples_skipped)).to eq 0

  expected: 0
       got: 2

...
New test introduced in this PR is flaky.
Datadog::Profiling::Collectors::ThreadContext SUSPENDED-skip optimization when a thread is suspended (does not hold the GVL) still records at least one sample per profile period via serialize from rspec (Fix with Cursor)
expected: 1
     got: 4

(compared using ==)

Failure/Error: expect(stats.fetch(:inactive_thread_samples_skipped)).to eq 1

  expected: 1
       got: 4

...
New test introduced in this PR is flaky.
View in Flaky Test Management

ℹ️ Info

No other issues found (see more)

🧪 All tests passed

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 89.87% (+0.02%)

Useful? React with 👍 / 👎

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

🔗 Commit SHA: 60714ad | Docs | Datadog PR Page | Give us feedback!}

pr-commenter · 2026-05-19T11:53:18Z

Benchmarks

Benchmark execution time: 2026-06-15 11:17:54

Comparing candidate commit ed9e438 in PR branch skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample with baseline commit 3f61acd in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 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 ----------------------------------'

eregon · 2026-05-20T17:42:44Z

@ivoanjo This is almost ready for a first pass review but needs tests.
Claude was also quite verbose in its comments, let me know what you think.

dd-octo-sts · 2026-05-21T13:28:11Z

Typing analysis

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06e3149986

ℹ️ 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".

ivoanjo

I haven't yet finished going through it all, but here's a first pass before I go for lunch :)

I think in general this looks reasonable and is in the right direction, all my notes are about the details of how we get there ;)

ivoanjo

One more pass :)

eregon · 2026-05-26T09:48:28Z

typedef int rb_internal_thread_specific_key_t;
#define RB_INTERNAL_THREAD_SPECIFIC_KEY_MAX 8
/**
 * Create a key to store thread specific data.
 *
 * These APIs are designed for tools using
 * rb_internal_thread_event_hook APIs.
 *
 * Note that only `RB_INTERNAL_THREAD_SPECIFIC_KEY_MAX` keys
 * can be created. raises `ThreadError` if exceeded.

That's one more reason to not use 2 rb_internal_thread_specific_key_t and use only 1 pointing to per_thread_context. Would also make it work on 3.2 which would be less ifdef-heavy.

* 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. * 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.

eregon · 2026-06-09T17:26:14Z

This is ready for another round of review

ivoanjo

Gave it another pass! Most of my feedback is very small, the big thing is the bias on the first sample after the period where we skipped.

ivoanjo

👍 LGTM. One last pass at feedback, none blocking -- e.g. I think it's OK to merge as-is and then address feedback in a follow-up PR.

The only important thing I'd like to raise (it wasn't clear to me) is validating the assumption that a postponed job scheduled inside a RUBY_INTERNAL_THREAD_EVENT_RESUMED callback will always run before the thread gets a chance to move. Just in case as getting that one wrong will mean very subtle bias that will be really hard to spot and really hard to debug.

ivoanjo

👍 Still good, (almost almost) no further questions, your honor 🎉

eregon · 2026-06-15T09:36:29Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0be09838e4

ℹ️ 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".

* Since such threads did not get the GVL then their Ruby-level stack must be the same and there is no need to sample them, we can just adjust the duration of the next sample instead. We need to sample when the GVL is acquired again, otherwise we would report an incorrect stack for the sample representing the period without GVL. * Emit a catch-up sample for still-suspended threads before serialization so we still sample at least once per reporting period (60 seconds). * The per-Ruby-Thread state is an integer: - low bit: current state (1 = suspended/no-GVL, 0 = running/has-GVL) - bits 1+: monotonic event counter, incremented on every RESUMED for that Thread * Add benchmark for profiling overhead. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>

The counter is global across all threads, so background threads in the test process can also be skipped, making exact equality checks flaky. The per-thread was_skipped_at_last_sample assertions already validate the optimization correctly. Co-Authored-By: Claude Opus 4.6 <[email protected]>

Skipped threads intentionally keep a stale wall_time_at_previous_sample_ns, so exclude them from the assertion that checks all threads have a wall time within the current sample window. Co-Authored-By: Claude Opus 4.6 <[email protected]>

serialize! can flush extra samples for skipped background threads. Use sample_for_thread in single_sample to select the expected thread's sample. For #sample_skipped_allocation_samples, filter by alloc-samples since it uses a synthetic thread. Co-Authored-By: Claude Opus 4.6 <[email protected]>

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

eregon commented May 19, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from d838099 to 694a7bc Compare May 19, 2026 11:27

eregon commented May 19, 2026

View reviewed changes

Comment thread benchmarks/profiling_sample_overhead.rb Outdated

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch 4 times, most recently from 81f223e to f9f4da7 Compare May 20, 2026 17:40

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from f9f4da7 to f000bc7 Compare May 20, 2026 18:09

eregon marked this pull request as ready for review May 21, 2026 13:23

eregon requested review from a team as code owners May 21, 2026 13:23

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from 06e3149 to e4fbc56 Compare May 21, 2026 13:27

dd-octo-sts Bot added the core Involves Datadog core libraries label May 21, 2026

chatgpt-codex-connector Bot reviewed May 21, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c Outdated

ivoanjo reviewed May 22, 2026

View reviewed changes

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from e4fbc56 to 6bb2090 Compare May 26, 2026 09:51

eregon mentioned this pull request May 27, 2026

Store per_thread_context in each Ruby Thread #5816

Merged

ivoanjo reviewed Jun 10, 2026

View reviewed changes

Comment thread spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb Outdated

Comment thread benchmarks/profiling_sample_overhead.rb

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from cbd3d5a to ba5360b Compare June 10, 2026 16:20

eregon requested a review from ivoanjo June 11, 2026 10:31

ivoanjo approved these changes Jun 11, 2026

View reviewed changes

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch 4 times, most recently from c63c8a7 to 484b6ea Compare June 12, 2026 16:03

ivoanjo approved these changes Jun 12, 2026

View reviewed changes

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

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch 2 times, most recently from 180c7cd to 0be0983 Compare June 15, 2026 09:35

chatgpt-codex-connector Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c Outdated

eregon commented Jun 15, 2026

View reviewed changes

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

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch 2 times, most recently from 1c5da8c to 51a5f99 Compare June 15, 2026 10:11

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from 51a5f99 to ed9e438 Compare June 15, 2026 10:52

eregon and others added 2 commits June 15, 2026 13:23

eregon enabled auto-merge June 15, 2026 11:29

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from 039406e to 04e1673 Compare June 15, 2026 12:09

eregon force-pushed the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch from 04e1673 to 60714ad Compare June 15, 2026 12:18

eregon merged commit 7a07876 into master Jun 15, 2026
589 checks passed

eregon deleted the skip-sampling-threads-which-did-not-acquire-the-gvl-since-last-sample branch June 15, 2026 12:39

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

ivoanjo mentioned this pull request Jun 17, 2026

Experiments to reduce profiling overhead #5751

Closed

Uh oh!

Conversation

eregon commented May 19, 2026

Uh oh!

dd-octo-sts Bot commented May 19, 2026

Uh oh!

Uh oh!

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

Uh oh!

⚠️ Warnings

ℹ️ Info

Uh oh!

Uh oh!

pr-commenter Bot commented May 19, 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

Uh oh!

eregon commented May 20, 2026

Uh oh!

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

Uh oh!

Typing analysis

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

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!

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!

eregon commented May 26, 2026

Uh oh!

eregon commented Jun 9, 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!

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!

datadog-prod-us1-3 Bot commented May 19, 2026 •

edited by datadog-datadog-prod-us1 Bot

Loading

pr-commenter Bot commented May 19, 2026 •

edited

Loading

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

edited

Loading