Skip to content

Commit 55dea0e

Browse files
committed
Simplify units in Duration/Instant math on Windows
Right now we do unit conversions between PerfCounter measurements and nanoseconds for every add/sub we do between Durations and Instants on Windows machines. This leads to goofy behavior, like this snippet failing: ``` let now = Instant::now(); let offset = Duration::from_millis(5); assert_eq!((now + offset) - now, (now - now) + offset); ``` with precision problems like this: ``` thread 'main' panicked at 'assertion failed: `(left == right)` left: `4.999914ms`, right: `5ms`', src\main.rs:6:5 ``` To fix it, this changeset does the unit conversion once, when we measure the clock, and all the subsequent math in u64 nanoseconds. It also adds an exact associativity test to the `sys/time.rs` test suite to make sure we don't regress on this in the future.
1 parent 6bba352 commit 55dea0e

File tree

2 files changed

+87
-42
lines changed

2 files changed

+87
-42
lines changed

src/libstd/sys/windows/time.rs

Lines changed: 78 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
use cmp::Ordering;
22
use fmt;
33
use mem;
4-
use sync::Once;
54
use sys::c;
6-
use sys::cvt;
7-
use sys_common::mul_div_u64;
85
use time::Duration;
96
use convert::TryInto;
107
use core::hash::{Hash, Hasher};
@@ -14,7 +11,7 @@ const INTERVALS_PER_SEC: u64 = NANOS_PER_SEC / 100;
1411

1512
#[derive(Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Debug, Hash)]
1613
pub struct Instant {
17-
t: c::LARGE_INTEGER,
14+
t: u64,
1815
}
1916

2017
#[derive(Copy, Clone)]
@@ -33,11 +30,12 @@ pub const UNIX_EPOCH: SystemTime = SystemTime {
3330

3431
impl Instant {
3532
pub fn now() -> Instant {
36-
let mut t = Instant { t: 0 };
37-
cvt(unsafe {
38-
c::QueryPerformanceCounter(&mut t.t)
39-
}).unwrap();
40-
t
33+
// High precision timing on windows operates in "Performance Counter"
34+
// units, as returned by the WINAPI QueryPerformanceCounter function.
35+
// These relate to seconds by a factor of QueryPerformanceFrequency.
36+
// In order to keep unit conversions out of normal interval math, we
37+
// measure in QPC units and immediately convert to nanoseconds.
38+
perf_counter::PerformanceCounterInstant::now().into()
4139
}
4240

4341
pub fn actually_monotonic() -> bool {
@@ -49,43 +47,36 @@ impl Instant {
4947
}
5048

5149
pub fn sub_instant(&self, other: &Instant) -> Duration {
52-
// Values which are +- 1 need to be considered as basically the same
53-
// units in time due to various measurement oddities, according to
54-
// Windows [1]
55-
//
56-
// [1]:
57-
// https://msdn.microsoft.com/en-us/library/windows/desktop
58-
// /dn553408%28v=vs.85%29.aspx#guidance
59-
if other.t > self.t && other.t - self.t == 1 {
50+
// On windows there's a threshold below which we consider two timestamps
51+
// equivalent due to measurement error. For more details + doc link,
52+
// check the docs on epsilon_nanos.
53+
let epsilon_ns =
54+
perf_counter::PerformanceCounterInstant::epsilon_nanos() as u64;
55+
if other.t > self.t && other.t - self.t <= epsilon_ns {
6056
return Duration::new(0, 0)
6157
}
62-
let diff = (self.t as u64).checked_sub(other.t as u64)
63-
.expect("specified instant was later than \
64-
self");
65-
let nanos = mul_div_u64(diff, NANOS_PER_SEC, frequency() as u64);
66-
Duration::new(nanos / NANOS_PER_SEC, (nanos % NANOS_PER_SEC) as u32)
58+
let diff = (self.t).checked_sub(other.t)
59+
.expect("specified instant was later than self");
60+
Duration::new(diff / NANOS_PER_SEC, (diff % NANOS_PER_SEC) as u32)
6761
}
6862

6963
pub fn checked_add_duration(&self, other: &Duration) -> Option<Instant> {
70-
let freq = frequency() as u64;
71-
let t = other.as_secs()
72-
.checked_mul(freq)?
73-
.checked_add(mul_div_u64(other.subsec_nanos() as u64, freq, NANOS_PER_SEC))?
64+
let sum = other.as_secs()
65+
.checked_mul(NANOS_PER_SEC)?
66+
.checked_add(other.subsec_nanos() as u64)?
7467
.checked_add(self.t as u64)?;
7568
Some(Instant {
76-
t: t as c::LARGE_INTEGER,
69+
t: sum,
7770
})
7871
}
7972

8073
pub fn checked_sub_duration(&self, other: &Duration) -> Option<Instant> {
81-
let freq = frequency() as u64;
82-
let t = other.as_secs().checked_mul(freq).and_then(|i| {
83-
(self.t as u64).checked_sub(i)
84-
}).and_then(|i| {
85-
i.checked_sub(mul_div_u64(other.subsec_nanos() as u64, freq, NANOS_PER_SEC))
86-
})?;
74+
let other_ns = other.as_secs()
75+
.checked_mul(NANOS_PER_SEC)?
76+
.checked_add(other.subsec_nanos() as u64)?;
77+
let difference = self.t.checked_sub(other_ns)?;
8778
Some(Instant {
88-
t: t as c::LARGE_INTEGER,
79+
t: difference,
8980
})
9081
}
9182
}
@@ -186,14 +177,59 @@ fn intervals2dur(intervals: u64) -> Duration {
186177
((intervals % INTERVALS_PER_SEC) * 100) as u32)
187178
}
188179

189-
fn frequency() -> c::LARGE_INTEGER {
190-
static mut FREQUENCY: c::LARGE_INTEGER = 0;
191-
static ONCE: Once = Once::new();
180+
mod perf_counter {
181+
use super::{NANOS_PER_SEC};
182+
use sync::Once;
183+
use sys_common::mul_div_u64;
184+
use sys::c;
185+
use sys::cvt;
186+
187+
pub struct PerformanceCounterInstant {
188+
ts: c::LARGE_INTEGER
189+
}
190+
impl PerformanceCounterInstant {
191+
pub fn now() -> Self {
192+
Self {
193+
ts: query()
194+
}
195+
}
192196

193-
unsafe {
194-
ONCE.call_once(|| {
195-
cvt(c::QueryPerformanceFrequency(&mut FREQUENCY)).unwrap();
196-
});
197-
FREQUENCY
197+
// Per microsoft docs, the margin of error for cross-thread time comparisons
198+
// using QueryPerformanceCounter is 1 "tick" -- defined as 1/frequency().
199+
// Reference: https://docs.microsoft.com/en-us/windows/desktop/SysInfo
200+
// /acquiring-high-resolution-time-stamps
201+
pub fn epsilon_nanos() -> u32 {
202+
let epsilon = NANOS_PER_SEC / (frequency() as u64);
203+
// As noted elsewhere, subsecond nanos always fit in a u32
204+
epsilon as u32
205+
}
206+
}
207+
impl From<PerformanceCounterInstant> for super::Instant {
208+
fn from(other: PerformanceCounterInstant) -> Self {
209+
let freq = frequency() as u64;
210+
Self {
211+
t: mul_div_u64(other.ts as u64, NANOS_PER_SEC, freq)
212+
}
213+
}
214+
}
215+
216+
fn frequency() -> c::LARGE_INTEGER {
217+
static mut FREQUENCY: c::LARGE_INTEGER = 0;
218+
static ONCE: Once = Once::new();
219+
220+
unsafe {
221+
ONCE.call_once(|| {
222+
cvt(c::QueryPerformanceFrequency(&mut FREQUENCY)).unwrap();
223+
});
224+
FREQUENCY
225+
}
226+
}
227+
228+
fn query() -> c::LARGE_INTEGER {
229+
let mut qpc_value: c::LARGE_INTEGER = 0;
230+
cvt(unsafe {
231+
c::QueryPerformanceCounter(&mut qpc_value)
232+
}).unwrap();
233+
qpc_value
198234
}
199235
}

src/libstd/time.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -610,6 +610,15 @@ mod tests {
610610
assert_eq!(a + year, a.checked_add(year).unwrap());
611611
}
612612

613+
#[test]
614+
fn instant_math_is_associative() {
615+
let now = Instant::now();
616+
let offset = Duration::from_millis(5);
617+
// Changing the order of instant math shouldn't change the results,
618+
// especially when the expression reduces to X + identity.
619+
assert_eq!((now + offset) - now, (now - now) + offset);
620+
}
621+
613622
#[test]
614623
#[should_panic]
615624
fn instant_duration_panic() {

0 commit comments

Comments
 (0)