Cleanups related to INVALID_TIME - #6248
Conversation
Since "Seed wall/cpu timestamps in initialize_context instead of mark_thread_as_profiler_internal", `wall_time_at_previous_sample_ns` is always seeded with a real clock value and is never set to INVALID_TIME, so that branch of `update_time_since_previous_sample` was dead for the wall-time path. `handle_gvl_waiting` already relies on this invariant, as it compares `wall_time_at_previous_sample_ns` without guarding for INVALID_TIME. Move the fix up into `update_cpu_time_since_previous_sample`, which is the only path where the baseline can be missing (when `cpu_time_now_ns` fails to read the clock). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`gc_tracking.cpu_time_at_start_ns` and `gc_tracking.wall_time_at_start_ns` are always written together in `on_gc_start` and cleared together in `on_gc_finish` and `initialize_context`, and neither `monotonic_wall_time_now_ns` nor `cpu_time_now_ns` can return INVALID_TIME (they return 0 on failure), so the two can never disagree. Check only the cpu-time field, which is what `update_time_since_previous_sample` already does for the same "is this thread in GC?" question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only `system_epoch_ns_reference` is ever tested to decide whether the monotonic-to-epoch state needs to be (re)computed; `delta_to_epoch_ns` is unconditionally overwritten at that point, so initializing it to INVALID_TIME just made it look like either field could be the marker. The designated initializer still zeroes the field, so this is not a behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`monotonic_wall_time_now_ns` signals failure by returning 0, not INVALID_TIME. `on_gc_finish` already bails out on a 0 end timestamp, but `on_gc_start` stored a failed read as-is, so the matching `on_gc_finish` would have computed a GC duration of "now minus the epoch" and accumulated it. Skip tracking the step instead, leaving the fields as INVALID_TIME so `on_gc_finish` ignores it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ff2d612 | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-08-27 12:49:11 Comparing candidate commit ff2d612 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
| // If our start timestamp is not OK, we don't start tracking this GC: leaving the fields as INVALID_TIME makes | ||
| // on_gc_finish skip it, rather than have it compute a bogus duration from a 0 start time | ||
| if (wall_time_at_start_ns == 0) return; |
There was a problem hiding this comment.
Minor: I'm not sure it's worth explaining INVALID_TIME, as there's already a return above and doesn't explain it either.
| // If our start timestamp is not OK, we don't start tracking this GC: leaving the fields as INVALID_TIME makes | |
| // on_gc_finish skip it, rather than have it compute a bogus duration from a 0 start time | |
| if (wall_time_at_start_ns == 0) return; | |
| // If our start timestamp is not OK, we skip tracking this GC as well | |
| if (wall_time_at_start_ns == 0) return; |
3 participants
Review commit-by-commit.
Some of the commits are arguable, no problem to drop them if we think they are not worth it.
What does this PR do?
A few cleanups following #6006 (comment)
Motivation:
Cleaner/simpler code.
Change log entry
None
Additional Notes:
How to test the change?