Skip to content

Commit

Permalink
Auto merge of #56988 - alexcrichton:monotonic-instant, r=sfackler
Browse files Browse the repository at this point in the history
std: Force `Instant::now()` to be monotonic

This commit is an attempt to force `Instant::now` to be monotonic
through any means possible. We tried relying on OS/hardware/clock
implementations, but those seem buggy enough that we can't rely on them
in practice. This commit implements the same hammer Firefox recently
implemented (noted in #56612) which is to just keep whatever the lastest
`Instant::now()` return value was in memory, returning that instead of
the OS looks like it's moving backwards.

Closes #48514
Closes #49281
cc #51648
cc #56560
Closes #56612
Closes #56940
  • Loading branch information
bors committed Jan 8, 2019
2 parents 7ad470c + 255a3f3 commit 2f19f8c
Show file tree
Hide file tree
Showing 7 changed files with 103 additions and 28 deletions.
17 changes: 2 additions & 15 deletions src/librustc/util/profiling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use session::config::Options;

use std::fs;
use std::io::{self, StderrLock, Write};
use std::time::{Duration, Instant};
use std::time::Instant;

macro_rules! define_categories {
($($name:ident,)*) => {
Expand Down Expand Up @@ -205,20 +205,7 @@ impl SelfProfiler {
}

fn stop_timer(&mut self) -> u64 {
let elapsed = if cfg!(windows) {
// On Windows, timers don't always appear to be monotonic (see #51648)
// which can lead to panics when calculating elapsed time.
// Work around this by testing to see if the current time is less than
// our recorded time, and if it is, just returning 0.
let now = Instant::now();
if self.current_timer >= now {
Duration::new(0, 0)
} else {
self.current_timer.elapsed()
}
} else {
self.current_timer.elapsed()
};
let elapsed = self.current_timer.elapsed();

self.current_timer = Instant::now();

Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/cloudabi/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ impl Instant {
}
}

pub fn actually_monotonic() -> bool {
true
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
let diff = self.t
.checked_sub(other.t)
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/redox/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ impl Instant {
Instant { t: now(syscall::CLOCK_MONOTONIC) }
}

pub const fn zero() -> Instant {
Instant { t: Timespec { t: syscall::TimeSpec { tv_sec: 0, tv_nsec: 0 } } }
}

pub fn actually_monotonic() -> bool {
false
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.t.sub_timespec(&other.t).unwrap_or_else(|_| {
panic!("specified instant was later than self")
Expand Down
40 changes: 28 additions & 12 deletions src/libstd/sys/unix/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ struct Timespec {
}

impl Timespec {
const fn zero() -> Timespec {
Timespec {
t: libc::timespec { tv_sec: 0, tv_nsec: 0 },
}
}

fn sub_timespec(&self, other: &Timespec) -> Result<Duration, Duration> {
if self >= other {
Ok(if self.t.tv_nsec >= other.t.tv_nsec {
Expand Down Expand Up @@ -128,19 +134,22 @@ mod inner {
}

pub const UNIX_EPOCH: SystemTime = SystemTime {
t: Timespec {
t: libc::timespec {
tv_sec: 0,
tv_nsec: 0,
},
},
t: Timespec::zero(),
};

impl Instant {
pub fn now() -> Instant {
Instant { t: unsafe { libc::mach_absolute_time() } }
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn actually_monotonic() -> bool {
true
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
let info = info();
let diff = self.t.checked_sub(other.t)
Expand Down Expand Up @@ -258,19 +267,26 @@ mod inner {
}

pub const UNIX_EPOCH: SystemTime = SystemTime {
t: Timespec {
t: libc::timespec {
tv_sec: 0,
tv_nsec: 0,
},
},
t: Timespec::zero(),
};

impl Instant {
pub fn now() -> Instant {
Instant { t: now(libc::CLOCK_MONOTONIC) }
}

pub const fn zero() -> Instant {
Instant {
t: Timespec::zero(),
}
}

pub fn actually_monotonic() -> bool {
(cfg!(target_os = "linux") && cfg!(target_arch = "x86_64")) ||
(cfg!(target_os = "linux") && cfg!(target_arch = "x86")) ||
false // last clause, used so `||` is always trailing above
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.t.sub_timespec(&other.t).unwrap_or_else(|_| {
panic!("specified instant was later than self")
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/wasm/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ impl Instant {
Instant(TimeSysCall::perform(TimeClock::Monotonic))
}

pub const fn zero() -> Instant {
Instant(Duration::from_secs(0))
}

pub fn actually_monotonic() -> bool {
false
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
self.0 - other.0
}
Expand Down
8 changes: 8 additions & 0 deletions src/libstd/sys/windows/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ impl Instant {
t
}

pub fn actually_monotonic() -> bool {
false
}

pub const fn zero() -> Instant {
Instant { t: 0 }
}

pub fn sub_instant(&self, other: &Instant) -> Duration {
// Values which are +- 1 need to be considered as basically the same
// units in time due to various measurement oddities, according to
Expand Down
42 changes: 41 additions & 1 deletion src/libstd/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@
#![stable(feature = "time", since = "1.3.0")]

use cmp;
use error::Error;
use fmt;
use ops::{Add, Sub, AddAssign, SubAssign};
use sys::time;
use sys_common::FromInner;
use sys_common::mutex::Mutex;

#[stable(feature = "time", since = "1.3.0")]
pub use core::time::Duration;
Expand Down Expand Up @@ -153,7 +155,45 @@ impl Instant {
/// ```
#[stable(feature = "time2", since = "1.8.0")]
pub fn now() -> Instant {
Instant(time::Instant::now())
let os_now = time::Instant::now();

// And here we come upon a sad state of affairs. The whole point of
// `Instant` is that it's monotonically increasing. We've found in the
// wild, however, that it's not actually monotonically increasing for
// one reason or another. These appear to be OS and hardware level bugs,
// and there's not really a whole lot we can do about them. Here's a
// taste of what we've found:
//
// * #48514 - OpenBSD, x86_64
// * #49281 - linux arm64 and s390x
// * #51648 - windows, x86
// * #56560 - windows, x86_64, AWS
// * #56612 - windows, x86, vm (?)
// * #56940 - linux, arm64
// * https://bugzilla.mozilla.org/show_bug.cgi?id=1487778 - a similar
// Firefox bug
//
// It simply seems that this it just happens so that a lot in the wild
// we're seeing panics across various platforms where consecutive calls
// to `Instant::now`, such as via the `elapsed` function, are panicking
// as they're going backwards. Placed here is a last-ditch effort to try
// to fix things up. We keep a global "latest now" instance which is
// returned instead of what the OS says if the OS goes backwards.
//
// To hopefully mitigate the impact of this though a few platforms are
// whitelisted as "these at least haven't gone backwards yet".
if time::Instant::actually_monotonic() {
return Instant(os_now)
}

static LOCK: Mutex = Mutex::new();
static mut LAST_NOW: time::Instant = time::Instant::zero();
unsafe {
let _lock = LOCK.lock();
let now = cmp::max(LAST_NOW, os_now);
LAST_NOW = now;
Instant(now)
}
}

/// Returns the amount of time elapsed from another instant to this one.
Expand Down

0 comments on commit 2f19f8c

Please sign in to comment.