Skip to content

[NO-TICKET] Profiling: Assume wall-time can't go backwards - #6021

Open
ivoanjo wants to merge 5 commits into
masterfrom
ivoanjo/assume-time-doesnt-go-backwards
Open

ivoanjo wants to merge 5 commits into
masterfrom
ivoanjo/assume-time-doesnt-go-backwards

Conversation

@ivoanjo

@ivoanjo ivoanjo commented Jul 10, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

This PR removes the max_of(elapsed_time_ns, 0) we had for wall-time, replacing it with an exception similar to the one we have for cpu-time.

In practice, it means that rather than "paving over"/"ignoring" weird behavior in the clock, it now becomes an explicit error state.

Update: This has been open for a while but should be good for review now. It's probably easier to review commit-by-commit.

Motivation:

In the past, we actually had this "if wall-time goes back it's an exception logic", and we changed it in
#2336 because we saw that CLOCK_MONOTONIC did go backwards on macOS.

Recently we stopped using CLOCK_MONOTONIC on macOS, replacing it with CLOCK_MONOTONIC_RAW (that's
#5994) and @eregon did a bunch of experiments in #2336 to show that this clock doesn't go backwards on macOS.

So we can tighten our logic. Our research shows that CLOCK_MONOTONIC does not have this issue on Linux, so there we expect to be fine.

Change log entry

None.

Additional Notes:

This PR is stacked atop #6016 as that PR also touches some of the same helpers, but it's otherwise conceptually independent.

#6016 has been merged so this is good to go.

This PR is stacked atop #6302 to avoid conflicts with tweaks done in that PR or the earlier #6364. They are otherwise independent.

All prior PRs merged, ready to go again!

How to test the change?

I've added test coverage for this.

@ivoanjo
ivoanjo requested review from a team as code owners July 10, 2026 10:37
@dd-octo-sts dd-octo-sts Bot added the profiling Involves Datadog profiling label Jul 10, 2026
@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
Contributor

Tests

✅ All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 90.36% (-0.02%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 53651d8 | Docs | View more details | Give us feedback!

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7bd9c26bcc

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

Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c
Comment thread ext/datadog_profiling_native_extension/collectors_thread_context.c
@ivoanjo

ivoanjo commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

Hmm the assertion seems to be triggering sometimes. Investigating...

@ivoanjo
ivoanjo marked this pull request as draft July 10, 2026 10:55
@pr-commenter

pr-commenter Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2026-07-13 15:08:14

Comparing candidate commit c0add03 in PR branch ivoanjo/assume-time-doesnt-go-backwards with baseline commit c30c2eb in branch master.

📊 Benchmarking dashboard

Found 1 performance improvements and 0 performance regressions! Performance is the same for 47 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 - intern mixed existing and new

  • 🟩 throughput [+1.829op/s; +2.409op/s] or [+6.165%; +8.119%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:tracing - trace.to_digest - Continue

  • unstable throughput [-1476.559op/s; +1649.393op/s] or [-5.154%; +5.757%]

Base automatically changed from ivoanjo/simplify-gc-tracking-logic-try2 to master July 13, 2026 07:58
@ivoanjo
ivoanjo force-pushed the ivoanjo/assume-time-doesnt-go-backwards branch from e548045 to c0add03 Compare July 13, 2026 14:41
**What does this PR do?**

This PR removes the `max_of(elapsed_time_ns, 0)` we had for wall-time,
replacing it with an exception similar to the one we have for cpu-time.

In practice, it means that rather than "paving over"/"ignoring" weird
behavior in the clock, it now becomes an explicit error state.

**Motivation:**

In the past, we actually had this "if wall-time goes back it's an
exception logic", and we changed it in
#2336 because we saw
that `CLOCK_MONOTONIC` did go backwards on macOS.

Recently we stopped using `CLOCK_MONOTONIC` on macOS, replacing it
with `CLOCK_MONOTONIC_RAW` (that's
#5994) and @eregon did a
bunch of experiments in #2336
to show that this clock doesn't go backwards on macOS.

So we can tighten our logic. Our research shows that `CLOCK_MONOTONIC`
does not have this issue on Linux, so there we expect to be fine.

**Additional Notes:**

N/A

**How to test the change?**

I've added test coverage for this.
In the previous commit, we added an assertion that wall-time never went
backwards.

That we know of, that's still correct BUT we were hitting that assertion
for a different reason: in some cases, we took a timestamp (e.g. at the
beginning of sampling) and then used that timestamp when updating the
wall-time for all threads.

The sharp edge is that when a thread did not previously had a context,
`get_or_create_context_for` would trigger the creation of a context, and
that creation was getting a more recent timestamp.

Thus this happened:

* Profiler picks a timestamp for this sample (let's call it t0)
* Profiler iterates threads and triggers context creation for
  threads that didn't have it. Those threads get a timestamp of
  t1 > t0
* Profiler tries to sample and still uses t0. The check will see
  that a context had time t1 and current time claimed by the profiler
  is t0, and trigger the assertion

To fix this issue, I've tweaked the context creation code so that now it
receives the timestamp to use. Thus in the situation above, the new
contexts get t0, not t1, and thus we're fine.

In situations where we don't have a timestamp, we still get the latest
clock.
Like the previous commit, our assumption of "wall-time doesn't go
backwards" actually clashed with GVL profiling, where the timestamp for
"Waiting for GVL" could be after the timestamp for the current sample
which would trigger the
"BUG: Unexpected wall time going backwards between samples" assertion.

TL;DR previously we were detecting/handling by checking for wall-time
being 0, but now we need to account for time being <= 0 (timestamp in
the future) AND make sure not to call
`update_wall_time_since_previous_sample` until we know we're good to go.
@ivoanjo
ivoanjo force-pushed the ivoanjo/assume-time-doesnt-go-backwards branch from c0add03 to 53651d8 Compare September 23, 2026 15:53
@ivoanjo
ivoanjo changed the base branch from master to ivoanjo/gc-stress-testcase September 23, 2026 15:54
Base automatically changed from ivoanjo/gc-stress-testcase to master September 23, 2026 16:19
@ivoanjo
ivoanjo marked this pull request as ready for review September 23, 2026 16:36
@ivoanjo
ivoanjo requested a review from eregon September 23, 2026 16:36

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

profiling Involves Datadog profiling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant