Repository navigation
Conversation
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 53651d8 | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
💡 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".
|
Hmm the assertion seems to be triggering sometimes. Investigating... |
BenchmarksBenchmark execution time: 2026-07-13 15:08:14 Comparing candidate commit c0add03 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 47 metrics, 1 unstable metrics.
|
e548045 to
c0add03
Compare
**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.
c0add03 to
53651d8
Compare
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_MONOTONICdid go backwards on macOS.Recently we stopped using
CLOCK_MONOTONICon macOS, replacing it withCLOCK_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_MONOTONICdoes 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.