Repository navigation
feat(tick): add configurable fast instant retrieval - #648
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 473 474 +1
Lines 45913 45963 +50
=======================================
+ Hits 45913 45963 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
crates/tick/src/simple_clock.rs:49
SystemFastis documented as reading lower-precision OS time, but the implementation only changes theInstantsource (whilesystem_time()still usesSystemTime::now()). Updating the variant doc comment will avoid misleading readers about what becomes lower precision.
/// Reads lower-precision OS time with lower overhead.
#[cfg(feature = "fast-instant")]
SystemFast,
crates/tick/src/fast_instant.rs:64
platform_time()unconditionally panics ifclock_gettime(CLOCK_MONOTONIC_COARSE)fails. That can bring down production code whenfast-instantis enabled on older/atypical Linux environments where_COARSEis unavailable. Consider falling back toCLOCK_MONOTONICbefore panicking.
let result = unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC_COARSE, timestamp.as_mut_ptr()) };
assert_eq!(
result,
0,
"CLOCK_MONOTONIC_COARSE must be available: {}",
crates/tick/src/fast_instant.rs:125
platform_time_is_nonzerocan be flaky on freshly booted systems (bothCLOCK_MONOTONIC_COARSEandGetTickCount64can legitimately return 0 near boot). A monotonic/non-decreasing assertion avoids this unnecessary failure mode.
#[test]
fn platform_time_is_nonzero() {
assert_ne!(platform_time(), Duration::ZERO);
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/tick/src/fast_instant.rs:108
repeated_calls_use_cached_instantis likely to be flaky: it assumes the coarse platform clock will return identical consecutive timestamps within 1,000 iterations, which is not guaranteed (e.g., if the platform clock resolution is 1ms and the loop body takes >1ms per iteration). This can cause spurious CI failures while not asserting a stable correctness property.
Consider rewriting this as a deterministic monotonicity check (or removing the test if caching is purely an optimization).
for _ in 0..1_000 {
let current = now();
if current == previous {
return;
}
crates/tick/src/fast_instant.rs:122
platform_time_is_nonzeroassumes the platform monotonic clock cannot returnDuration::ZERO, but0is a valid monotonic timestamp (e.g., right after boot). This makes the test encode an unnecessary and potentially flaky constraint.
A better invariant to assert is monotonicity (second call >= first).
#[test]
fn platform_time_is_nonzero() {
assert_ne!(platform_time(), Duration::ZERO);
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Regenerate the tick README to resolve the generated-file conflict. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/tick/src/fast_instant.rs:120
- Same here: prefer
.unwrap()over.expect(...)when joining a spawned test thread.
let second = std::thread::spawn(move || calibration.now_at(calibration.platform_epoch + Duration::from_millis(2)))
.join()
.expect("test thread must complete");
crates/tick/src/fast_instant.rs:117
- In test code we generally prefer
.unwrap()over.expect(...)forJoinHandle::join(). The panic backtrace already points at the failing thread join, and this keeps test style consistent across the repo.
This issue also appears on line 118 of the same file.
let first = std::thread::spawn(move || calibration.now_at(calibration.platform_epoch + Duration::from_millis(1)))
.join()
.expect("test thread must complete");
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/tick/src/fast_instant.rs:40
- The
checked_add(...).expect("a monotonic platform timestamp cannot exceed the range of Instant")message is not a valid justification: monotonicity does not imply the value is withinInstant’s representable range. If this is intended to be an impossible condition, the expect message should explain the real invariant (e.g., thatelapsedis bounded by process uptime / platform counter range, so overflow would require implausibly long uptime).
.expect("a monotonic platform timestamp cannot exceed the range of Instant")
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 2 findings. One finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Hit |
Remove the unsubstantiated same-crate inline hint from the fast instant helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
Preserve the independently merged main changes before pushing review fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/tick/src/fast_instant.rs:56
clock_gettime(CLOCK_MONOTONIC_COARSE, ...)can fail for reasons other than the clock ID not existing (e.g., sandbox/seccomp restrictions). Panicking here turns an optional performance feature into a process-aborting reliability risk. Consider handling a non-zero return by falling back toInstant::now()for that call (or disabling the fast path after a failed calibration) instead of asserting.
let result = unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC_COARSE, timestamp.as_mut_ptr()) };
assert_eq!(result, 0, "CLOCK_MONOTONIC_COARSE predates Rust's minimum supported Linux kernel");
Follow the crate's any(feature, test) gating convention and provide target-specific dev dependencies for the platform clocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
crates/tick/src/fast_instant.rs:56
platform_time()panics the entire process ifclock_gettime(CLOCK_MONOTONIC_COARSE, ...)fails (e.g., due to seccomp/sandboxing). Since this is an optional performance feature, it’s safer to fall back to a more widely supported monotonic clock rather than crashing.
let result = unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC_COARSE, timestamp.as_mut_ptr()) };
assert_eq!(result, 0, "CLOCK_MONOTONIC_COARSE predates Rust's minimum supported Linux kernel");
crates/tick/src/periodic_timer.rs:210
- This test only asserts that
timer.next()completes before the timeout, but it would also pass if the stream unexpectedly returnedNone. SincePeriodicTimeris documented to never complete, the test should assert the item isSome(())to avoid masking regressions.
tokio::time::timeout(Duration::from_secs(1), timer.next())
.await
.expect("timer scheduling uses the driver's precise time source");
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f6987d90-a184-4cb2-8d5a-99581cb6d507
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/tick/src/fast_instant.rs:56
- The panic message on
clock_gettimefailure is misleading:clock_gettime(CLOCK_MONOTONIC_COARSE, ...)can fail for reasons other than an unsupported kernel (e.g., sandboxing/seccomp or unexpected OS errors), so the current message could send debugging in the wrong direction. Consider using an error message that reflects the actual call that failed and the expected invariant.
assert_eq!(result, 0, "CLOCK_MONOTONIC_COARSE predates Rust's minimum supported Linux kernel");
Adds an optional per-clock fast instant source behind the
fast-instantfeature without changing default behavior.Clock::with_fast_instant(self, enabled)andSimpleClock::with_fast_instant(self, enabled)configure the existinginstant()path. This lets callers keep a precise clock and create an independently configured fast clone; stopwatches created from each clone inherit its source. Timer scheduling remains on the clock driver's precise time source soDelayandPeriodicTimercannot miss a wake when coarse retrieval lags. Controlled clocks are unaffected. On Linux, fast retrieval usesCLOCK_MONOTONIC_COARSE; on Windows, it usesGetTickCount64; other targets delegate tostd::time::Instant::now(). Fast retrieval uses one process-wide calibration, keeping values comparable across threads without per-clock state, thread-local state, or caching. The implementation is private to tick and uses only target-specific optional FFI dependencies. The Criterion benchmark compares the public precise and configured-fast paths. A current Windows run measured approximately 29.75 ns for precise retrieval and 4.74 ns for fast retrieval, about 6.3x faster. Validation: tick all-feature and feature-disabled tests, all-feature doctests, Clippy with warnings denied, pinned Miri, formatting, generated README, and spelling pass.cargo packageremains blocked by the existing publishedthread_aware 0.8.0feature mismatch.