mathieu/granite-rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Rollup merge of #89926 - the8472:saturate-instant, r=Mark-Simulacrum

Browse source 
make `Instant::{duration_since, elapsed, sub}` saturating and remove workarounds

This removes all mutex/atomic-based workarounds for non-monotonic clocks and makes the previously panicking methods saturating instead. Additionally `saturating_duration_since` becomes deprecated since `duration_since` now fills that role.

Effectively this moves the fixup from `Instant` construction to the comparisons.

This has some observable effects, especially on platforms without monotonic clocks:

* Incorrectly ordered Instant comparisons no longer panic in release mode. This could hide some programming errors, but since debug mode still panics tests can still catch them.
* `checked_duration_since` will now return `None` in more cases. Previously it only happened when one compared instants obtained in the wrong order or manually created ones. Now it also does on backslides.
* non-monotonic intervals will not be transitive, i.e. `b.duration_since(a) + c.duration_since(b) != c.duration_since(a)`

The upsides are reduced complexity and lower overhead of `Instant::now`.

## Motivation

Currently we must choose between two poisons. One is high worst-case latency and jitter of `Instant::now()` due to explicit synchronization; see #83093 for benchmarks, the worst-case overhead is > 100x. The other is sporadic panics on specific, rare combinations of CPU/hypervisor/operating system due to platform bugs.

Use-cases where low-overhead, fine-grained timestamps are needed - such as syscall tracing, performance profiles or sensor data acquisition (drone flight controllers were mentioned in a libs meeting) in multi-threaded programs - are negatively impacted by the synchronization.

The panics are user-visible (program crashes), hard to reproduce and can be triggered by any dependency that might be using Instants for any reason.

A solution that is fast _and_ doesn't panic is desirable.

----

closes #84448
closes #86470
This commit is contained in:
Matthias Krüger 2022-02-13 06:44:12 +01:00 • committed by GitHub
commit 92613a25fc
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 57 additions and 267 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
library/std/src/sys/hermit/time.rs
View file

@ -115,14 +115,6 @@ impl Instant {
Instant { t: time }
}
pub const fn zero() -> Instant {
Instant { t: Timespec::zero() }
}
pub fn actually_monotonic() -> bool {
true
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
self.t.sub_timespec(&other.t).ok()
}

9
library/std/src/sys/itron/time.rs
View file

@ -14,15 +14,6 @@ impl Instant {
}
}
pub const fn zero() -> Instant {
Instant(0)
}
pub fn actually_monotonic() -> bool {
// There are ways to change the system time
false
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
self.0.checked_sub(other.0).map(|ticks| {
// `SYSTIM` is measured in microseconds

8
library/std/src/sys/sgx/time.rs
View file

@ -25,14 +25,6 @@ impl Instant {
pub fn checked_sub_duration(&self, other: &Duration) -> Option<Instant> {
Some(Instant(self.0.checked_sub(*other)?))
}
pub fn actually_monotonic() -> bool {
false
}
pub const fn zero() -> Instant {
Instant(Duration::from_secs(0))
}
}
impl SystemTime {

19
library/std/src/sys/unix/time.rs
View file

@ -154,14 +154,6 @@ mod inner {
Instant { t: unsafe { mach_absolute_time() } }
}
pub const fn zero() -> Instant {
Instant { t: 0 }
}
pub fn actually_monotonic() -> bool {
true
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
let diff = self.t.checked_sub(other.t)?;
let info = info();
@ -296,17 +288,6 @@ mod inner {
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"))
|| (cfg!(target_os = "linux") && cfg!(target_arch = "aarch64"))
|| cfg!(target_os = "fuchsia")
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
self.t.sub_timespec(&other.t).ok()
}

8
library/std/src/sys/unsupported/time.rs
View file

@ -13,14 +13,6 @@ impl Instant {
panic!("time not implemented on this platform")
}
pub const fn zero() -> Instant {
Instant(Duration::from_secs(0))
}
pub fn actually_monotonic() -> bool {
false
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
self.0.checked_sub(other.0)
}

8
library/std/src/sys/wasi/time.rs
View file

@ -25,14 +25,6 @@ impl Instant {
Instant(current_time(wasi::CLOCKID_MONOTONIC))
}
pub const fn zero() -> Instant {
Instant(Duration::from_secs(0))
}
pub fn actually_monotonic() -> bool {
true
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
self.0.checked_sub(other.0)
}

8
library/std/src/sys/windows/time.rs
View file

@ -41,14 +41,6 @@ impl Instant {
perf_counter::PerformanceCounterInstant::now().into()
}
pub fn actually_monotonic() -> bool {
false
}
pub const fn zero() -> Instant {
Instant { t: Duration::from_secs(0) }
}
pub fn checked_sub_instant(&self, other: &Instant) -> Option<Duration> {
// On windows there's a threshold below which we consider two timestamps
// equivalent due to measurement error. For more details + doc link,

109
library/std/src/time.rs
View file

@ -31,7 +31,6 @@
#![stable(feature = "time", since = "1.3.0")]
mod monotonic;
#[cfg(test)]
mod tests;
@ -50,8 +49,8 @@ pub use core::time::FromFloatSecsError;
/// A measurement of a monotonically nondecreasing clock.
/// Opaque and useful only with [`Duration`].
///
/// Instants are always guaranteed to be no less than any previously measured
/// instant when created, and are often useful for tasks such as measuring
/// Instants are always guaranteed, barring [platform bugs], to be no less than any previously
/// measured instant when created, and are often useful for tasks such as measuring
/// benchmarks or timing how long an operation takes.
///
/// Note, however, that instants are **not** guaranteed to be **steady**. In other
@ -84,6 +83,8 @@ pub use core::time::FromFloatSecsError;
/// }
/// ```
///
/// [platform bugs]: Instant#monotonicity
///
/// # OS-specific behaviors
///
/// An `Instant` is a wrapper around system-specific types and it may behave
@ -125,6 +126,26 @@ pub use core::time::FromFloatSecsError;
/// > structure cannot represent the new point in time.
///
/// [`add`]: Instant::add
///
/// ## Monotonicity
///
/// On all platforms `Instant` will try to use an OS API that guarantees monotonic behavior
/// if available, which is the case for all [tier 1] platforms.
/// In practice such guarantees are – under rare circumstances – broken by hardware, virtualization
/// or operating system bugs. To work around these bugs and platforms not offering monotonic clocks
/// [`duration_since`], [`elapsed`] and [`sub`] saturate to zero. In older Rust versions this
/// lead to a panic instead. [`checked_duration_since`] can be used to detect and handle situations
/// where monotonicity is violated, or `Instant`s are subtracted in the wrong order.
///
/// This workaround obscures programming errors where earlier and later instants are accidentally
/// swapped. For this reason future rust versions may reintroduce panics.
///
/// [tier 1]: https://doc.rust-lang.org/rustc/platform-support.html
/// [`duration_since`]: Instant::duration_since
/// [`elapsed`]: Instant::elapsed
/// [`sub`]: Instant::sub
/// [`checked_duration_since`]: Instant::checked_duration_since
///
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[stable(feature = "time2", since = "1.8.0")]
pub struct Instant(time::Instant);
@ -247,59 +268,19 @@ impl Instant {
#[must_use]
#[stable(feature = "time2", since = "1.8.0")]
pub fn now() -> Instant {
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 seems that this just happens 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, a few platforms are
// excluded as "these at least haven't gone backwards yet".
//
// While issues have been seen on arm64 platforms the Arm architecture
// requires that the counter monotonically increases and that it must
// provide a uniform view of system time (e.g. it must not be possible
// for a core to receive a message from another core with a time stamp
// and observe time going backwards (ARM DDI 0487G.b D11.1.2). While
// there have been a few 64bit SoCs that have bugs which cause time to
// not monoticially increase, these have been fixed in the Linux kernel
// and we shouldn't penalize all Arm SoCs for those who refuse to
// update their kernels:
// SUN50I_ERRATUM_UNKNOWN1 - Allwinner A64 / Pine A64 - fixed in 5.1
// FSL_ERRATUM_A008585 - Freescale LS2080A/LS1043A - fixed in 4.10
// HISILICON_ERRATUM_161010101 - Hisilicon 1610 - fixed in 4.11
// ARM64_ERRATUM_858921 - Cortex A73 - fixed in 4.12
if time::Instant::actually_monotonic() {
return Instant(os_now);
}
Instant(monotonic::monotonize(os_now))
Instant(time::Instant::now())
}
/// Returns the amount of time elapsed from another instant to this one.
/// Returns the amount of time elapsed from another instant to this one,
/// or zero duration if that instant is later than this one.
///
/// # Panics
///
/// This function will panic if `earlier` is later than `self`.
/// Previous rust versions panicked when `earlier` was later than `self`. Currently this
/// method saturates. Future versions may reintroduce the panic in some circumstances.
/// See [Monotonicity].
///
/// [Monotonicity]: Instant#monotonicity
///
/// # Examples
///
@ -311,16 +292,22 @@ impl Instant {
/// sleep(Duration::new(1, 0));
/// let new_now = Instant::now();
/// println!("{:?}", new_now.duration_since(now));
/// println!("{:?}", now.duration_since(new_now)); // 0ns
/// ```
#[must_use]
#[stable(feature = "time2", since = "1.8.0")]
pub fn duration_since(&self, earlier: Instant) -> Duration {
self.0.checked_sub_instant(&earlier.0).expect("supplied instant is later than self")
self.checked_duration_since(earlier).unwrap_or_default()
}
/// Returns the amount of time elapsed from another instant to this one,
/// or None if that instant is later than this one.
///
/// Due to [monotonicity bugs], even under correct logical ordering of the passed `Instant`s,
/// this method can return `None`.
///
/// [monotonicity bugs]: Instant#monotonicity
///
/// # Examples
///
/// ```no_run
@ -364,9 +351,11 @@ impl Instant {
///
/// # Panics
///
/// This function may panic if the current time is earlier than this
/// instant, which is something that can happen if an `Instant` is
/// produced synthetically.
/// Previous rust versions panicked when self was earlier than the current time. Currently this
/// method returns a Duration of zero in that case. Future versions may reintroduce the panic.
/// See [Monotonicity].
///
/// [Monotonicity]: Instant#monotonicity
///
/// # Examples
///
@ -442,6 +431,16 @@ impl SubAssign<Duration> for Instant {
impl Sub<Instant> for Instant {
type Output = Duration;
/// Returns the amount of time elapsed from another instant to this one,
/// or zero duration if that instant is later than this one.
///
/// # Panics
///
/// Previous rust versions panicked when `other` was later than `self`. Currently this
/// method saturates. Future versions may reintroduce the panic in some circumstances.
/// See [Monotonicity].
///
/// [Monotonicity]: Instant#monotonicity
fn sub(self, other: Instant) -> Duration {
self.duration_since(other)
}

116
library/std/src/time/monotonic.rs
View file

@ -1,116 +0,0 @@
use crate::sys::time;
#[inline]
pub(super) fn monotonize(raw: time::Instant) -> time::Instant {
inner::monotonize(raw)
}
#[cfg(any(all(target_has_atomic = "64", not(target_has_atomic = "128")), target_arch = "aarch64"))]
pub mod inner {
use crate::sync::atomic::AtomicU64;
use crate::sync::atomic::Ordering::*;
use crate::sys::time;
use crate::time::Duration;
pub(in crate::time) const ZERO: time::Instant = time::Instant::zero();
// bits 30 and 31 are never used since the nanoseconds part never exceeds 10^9
const UNINITIALIZED: u64 = 0b11 << 30;
static MONO: AtomicU64 = AtomicU64::new(UNINITIALIZED);
#[inline]
pub(super) fn monotonize(raw: time::Instant) -> time::Instant {
monotonize_impl(&MONO, raw)
}
#[inline]
pub(in crate::time) fn monotonize_impl(mono: &AtomicU64, raw: time::Instant) -> time::Instant {
let delta = raw.checked_sub_instant(&ZERO).unwrap();
let secs = delta.as_secs();
// occupies no more than 30 bits (10^9 seconds)
let nanos = delta.subsec_nanos() as u64;
// This wraps around every 136 years (2^32 seconds).
// To detect backsliding we use wrapping arithmetic and declare forward steps smaller
// than 2^31 seconds as expected and everything else as a backslide which will be
// monotonized.
// This could be a problem for programs that call instants at intervals greater
// than 68 years. Interstellar probes may want to ensure that actually_monotonic() is true.
let packed = (secs << 32) | nanos;
let updated = mono.fetch_update(Relaxed, Relaxed, |old| {
(old == UNINITIALIZED || packed.wrapping_sub(old) < u64::MAX / 2).then_some(packed)
});
match updated {
Ok(_) => raw,
Err(newer) => {
// Backslide occurred. We reconstruct monotonized time from the upper 32 bit of the
// passed in value and the 64bits loaded from the atomic
let seconds_lower = newer >> 32;
let mut seconds_upper = secs & 0xffff_ffff_0000_0000;
if secs & 0xffff_ffff > seconds_lower {
// Backslide caused the lower 32bit of the seconds part to wrap.
// This must be the case because the seconds part is larger even though
// we are in the backslide branch, i.e. the seconds count should be smaller or equal.
//
// We assume that backslides are smaller than 2^32 seconds
// which means we need to add 1 to the upper half to restore it.
//
// Example:
// most recent observed time: 0xA1_0000_0000_0000_0000u128
// bits stored in AtomicU64: 0x0000_0000_0000_0000u64
// backslide by 1s
// caller time is 0xA0_ffff_ffff_0000_0000u128
// -> we can fix up the upper half time by adding 1 << 32
seconds_upper = seconds_upper.wrapping_add(0x1_0000_0000);
}
let secs = seconds_upper | seconds_lower;
let nanos = newer as u32;
ZERO.checked_add_duration(&Duration::new(secs, nanos)).unwrap()
}
}
}
}
#[cfg(all(target_has_atomic = "128", not(target_arch = "aarch64")))]
pub mod inner {
use crate::sync::atomic::AtomicU128;
use crate::sync::atomic::Ordering::*;
use crate::sys::time;
use crate::time::Duration;
const ZERO: time::Instant = time::Instant::zero();
static MONO: AtomicU128 = AtomicU128::new(0);
#[inline]
pub(super) fn monotonize(raw: time::Instant) -> time::Instant {
let delta = raw.checked_sub_instant(&ZERO).unwrap();
// Split into seconds and nanos since Duration doesn't have a
// constructor that takes a u128
let secs = delta.as_secs() as u128;
let nanos = delta.subsec_nanos() as u128;
let timestamp: u128 = secs << 64 | nanos;
let timestamp = MONO.fetch_max(timestamp, Relaxed).max(timestamp);
let secs = (timestamp >> 64) as u64;
let nanos = timestamp as u32;
ZERO.checked_add_duration(&Duration::new(secs, nanos)).unwrap()
}
}
#[cfg(not(any(target_has_atomic = "64", target_has_atomic = "128")))]
pub mod inner {
use crate::cmp;
use crate::sys::time;
use crate::sys_common::mutex::StaticMutex;
#[inline]
pub(super) fn monotonize(os_now: time::Instant) -> time::Instant {
static LOCK: StaticMutex = StaticMutex::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;
now
}
}
}

31
library/std/src/time/tests.rs
View file

@ -90,10 +90,9 @@ fn instant_math_is_associative() {
}
#[test]
#[should_panic]
fn instant_duration_since_panic() {
fn instant_duration_since_saturates() {
let a = Instant::now();
let _ = (a - Duration::SECOND).duration_since(a);
assert_eq!((a - Duration::SECOND).duration_since(a), Duration::ZERO);
}
#[test]
@ -109,6 +108,7 @@ fn instant_checked_duration_since_nopanic() {
#[test]
fn instant_saturating_duration_since_nopanic() {
let a = Instant::now();
#[allow(deprecated, deprecated_in_future)]
let ret = (a - Duration::SECOND).saturating_duration_since(a);
assert_eq!(ret, Duration::ZERO);
}
@ -192,31 +192,6 @@ fn since_epoch() {
assert!(a < hundred_twenty_years);
}
#[cfg(all(target_has_atomic = "64", not(target_has_atomic = "128")))]
#[test]
fn monotonizer_wrapping_backslide() {
use super::monotonic::inner::{monotonize_impl, ZERO};
use core::sync::atomic::AtomicU64;
let reference = AtomicU64::new(0);
let time = match ZERO.checked_add_duration(&Duration::from_secs(0xffff_ffff)) {
Some(time) => time,
None => {
// platform cannot represent u32::MAX seconds so it won't have to deal with this kind
// of overflow either
return;
}
};
let monotonized = monotonize_impl(&reference, time);
let expected = ZERO.checked_add_duration(&Duration::from_secs(1 << 32)).unwrap();
assert_eq!(
monotonized, expected,
"64bit monotonizer should handle overflows in the seconds part"
);
}
macro_rules! bench_instant_threaded {
($bench_name:ident, $thread_count:expr) => {
#[bench]