use CLOCK_BOOTTIME in Instant::now #88714

hellow554 · 2021-09-07T07:42:44Z

There was a discussion on #87907 that all platforms expect
linux advance its monotonic timer if the system goes into a sleep state
(e.g. suspend or hibernate).
It was decided that CLOCK_BOOTTIME should be used on inux targets, but
because that constant would break older systems (e.g. linux kernels
<2.6.39) there should be a fallback to CLOCK_MONOTONIC.

Related to #87907
Closes #87906

cc @joshtriplett @cuviper

rust-highfive · 2021-09-07T07:42:47Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

hellow554 · 2021-09-07T07:44:08Z

library/std/src/sys/unix/time.rs

+            let t = match now(libc::CLOCK_BOOTTIME) {
+                Err(e) if e.kind() == io::ErrorKind::InvalidInput => now(libc::CLOCK_MONOTONIC),
+                v => v,
+            }
+            .unwrap();


IMHO this match is easier to read than a or_else, but I'm not sure if that's faster/better/stronger.

Here's a godbolt link, maybe someone can decide what to use: https://rust.godbolt.org/z/6Eoxc7PEz

At a glance, there is not what I would consider a significant enough difference in the assembly to merit one approach over another, which is as it should be for such similar expressions. One may be better contextually if carefully benchmarked.

hellow554 · 2021-09-07T07:44:51Z

library/std/src/sys/unix/time.rs

        let mut t = Timespec { t: libc::timespec { tv_sec: 0, tv_nsec: 0 } };
-        cvt(unsafe { libc::clock_gettime(clock, &mut t.t) }).unwrap();
-        t
+        cvt(unsafe { libc::clock_gettime(clock, &mut t.t) }).map(|_| t)


Should I write:

cvt(...)?; t

instead?

Yes, please.

CryZe · 2021-09-07T11:06:21Z

Alright, so the situation is the following:

All the resources I found seem to very roughly indicate that except for Linux CLOCK_MONOTONIC does actually include suspend on basically all / most of (?) the operating systems.
I have tried to verify this since then, but had a lot of trouble setting up FreeBSD for example (it refused to ever wake up again after suspending)
Therefore I am not super confident that the claim is actually true.
However if anything macOS would also need to switch over to CLOCK_MONOTONIC as it currently uses it's own implementation in std. This function that is called is: not recommended by the documentation, has a lower limit (u32 + u32 as opposed to u64 + u32) and doesn't include suspends.

So yeah, that's why I didn't open this PR yet.

hellow554 · 2021-09-07T11:18:56Z

macos currently uses mach_absolute_time (here) which states:

[...] this clock does not increment while the system is asleep.

and

Prefer to use the equivalent clock_gettime_nsec_np(CLOCK_UPTIME_RAW) in nanoseconds.

Following that: http://manpagez.com/man/3/clock_gettime_nsec_np/

CLOCK_MONOTONIC
clock that increments monotonically, tracking the time since an arbitrary point, and will continue to increment while the system is asleep.

CLOCK_MONOTONIC_RAW
clock that increments monotonically, tracking the time since an arbitrary point like CLOCK_MONOTONIC. However, this clock is unaffected by frequency or time adjustments. It should not be compared to other system time sources.

I'm more than willing to also add a change for that. Only problem I don't have a mac to test that.

thomcc · 2021-09-07T13:59:18Z

Essentially all the clock APIs on Darwin other than mach_absolute_time were added in Darwin 16 (macOS 10.12, iOS 10.0, ...). This includes clock_gettime, clock_gettime_nsec_np, mach_continuous_time, CLOCK_UPTIME_RAW and all the other CLOCK_* constants...

We support back to macOS 10.6 or 10.7, which is considerably older. Darwin 16 is only about 4 or 5 years old, and I think it would be a problem for libstd to require this.

workingjubilee · 2021-09-07T21:58:57Z

It would be a good idea if further metadiscussion took place in the original issue, since it seems to be that the discussion is ongoing. Discussion immediately relevant to this PR's code, of course, can continue here.

library/std/src/sys/unix/time.rs

joshtriplett · 2021-09-07T22:31:40Z

@thomcc I think it's reasonable to handle macOS in a separate PR. And as with this PR, it'd be acceptable if the primary approach required new macOS and the fallback didn't account for suspend time.

library/std/src/sys/unix/time.rs

jonhoo · 2021-09-23T23:12:24Z

Should this also be tagged with relnotes?

joshtriplett · 2021-10-04T02:55:25Z

This needs an update to make the use of CLOCK_BOOTTIME conditional (only on Linux), and to make the fallback conditional (only on x86, x86_64, powerpc, powerpc64, and s390x).

This also needs an update (as noticed by the author) to use ? in error handling.

JohnCSimon · 2021-10-19T04:38:04Z

Ping from triage:
@hellow554 Can you please post your status on this PR?

hellow554 · 2021-10-24T19:16:22Z

I'm sorry for the late response. Got many things to do.

I reworked the error handling in the now function and the fallback to CLOCK_MONOTIC is now only done on said architectures.

Linux is only used if I see it correctly because of the #[cfg] on the mod inner block, am I right?

joshtriplett · 2021-10-25T08:18:23Z

The mod inner here applies to anything that isn't macos or ios: #[cfg(not(any(target_os = "macos", target_os = "ios")))].

You need to add a cfg limiting the use of CLOCK_BOOTTIME to Linux only.

hellow554 · 2021-10-25T09:10:14Z

ALright, I wrapped it into if cfg! :)

maurer · 2022-09-08T20:16:54Z

Please add "android" to the list of CFG'd OSes for using CLOCK_BOOTTIME - we currently carry a patch to std in Android to use this clock for Instant.

I'm not on the libs team, but I see no reason we'd need a fallback path now that #95026 is merged - we no longer support any of the Linux systems where CLOCK_BOOTTIME does not exist.

cuviper · 2022-09-09T22:06:07Z

We don't need a fallback anymore -- we just need to decide if we want to change this after all.

faern · 2022-11-11T08:53:31Z

Can we please make this happen now? My impression is that we had agreed on switching to CLOCK_BOOTTIME, we just needed to fix the compatibility issues. And those are long gone now.

bors · 2022-11-19T00:12:31Z

☔ The latest upstream changes (presumably #104573) made this pull request unmergeable. Please resolve the merge conflicts.

JohnCSimon · 2022-11-27T01:26:21Z

@hellow554 - PR has merge conflicts

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

mati865 · 2022-11-27T07:56:42Z

We don't need a fallback anymore -- we just need to decide if we want to change this after all.

My impression is this PR is still waiting on the decision.

There was a discussion on rust-lang#87907 that all platforms expect linux advance its monotonic timer if the system goes into a sleep state (e.g. suspend or hibernate). It was decided that CLOCK_BOOTTIME should be used on unix targets.

rustbot · 2022-11-29T14:11:26Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

hellow554 · 2022-11-29T14:17:34Z

@rustbot ready

hellow554 · 2022-11-29T14:58:46Z

That's an error I can't explain. Maybe somebody can?

-    Instant {
-        t: Timespec::now(if cfg!(target_os = "linux") {
-            libc::CLOCK_BOOTTIME
-        } else {
-            libc::CLOCK_MONOTONIC
-        }),
-    }
+    #[cfg(target_os = "macos")]
+    const clock_id: libc::clockid_t = libc::CLOCK_UPTIME_RAW;
+
+    #[cfg(target_os = "linux")]
+    const clock_id: libc::clockid_t = libc::CLOCK_BOOTTIME;
+
+    #[cfg(not(any(target_os = "macos", target_os = "linux")))]
+    const clock_id: libc::clockid_t = libc::CLOCK_MONOTONIC;
+
+    Instant { t: Timespec::now(clock_id) }

that's not a lot of changes and certainly no changes that should do have an impact. Maybe something else changed in the meantime?

maniwani · 2022-11-29T16:17:37Z

It looks like Miri tests are failing when emulating x86_64-unknown-linux-gnu. I believe the error is that Miri doesn't recognize libc::CLOCK_BOOTTIME.

I think it needs to be added to the shims:
https://github.com/rust-lang/miri/blob/master/src/shims/time.rs

rustbot · 2022-11-30T06:21:24Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2022-11-30T07:43:13Z

There was a discussion on #87907 that all platforms expect
linux advance its monotonic timer if the system goes into a sleep state
(e.g. suspend or hibernate).

This does not seem accurate. Since #103594, macOS on aarch64 uses CLOCK_UPTIME_RAW which does not increment when the system is asleep. On x86 it uses mach_absolute_time which, according to that manpage, also does not increment when the system is asleep.

RalfJung · 2022-11-30T07:44:37Z

src/tools/miri/src/shims/time.rs

@@ -45,6 +45,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                relative_clocks = vec![
                    this.eval_libc_i32("CLOCK_MONOTONIC")?,
                    this.eval_libc_i32("CLOCK_MONOTONIC_COARSE")?,
+                    // This is the equivalent to `CLOCK_UPTIME_RAW` in the macos section
+                    // We support it, because std relies on it


This comment is backwards -- BOOTTIME you say does increment when the machine is asleep, but CLOCK_UPTIME_RAW does not.

If anything, it seems like CLOCK_BOOTTIME on linux is like CLOCK_MONOTONIC on macOS, and CLOCK_MONOTONIC on linux is like CLOCK_UPTIME_RAW (and mach_absolute_time) on macos?

Yeah judging from a quick search, looks like MONOTONIC on Linux does not increment when sleeping, but on macos it does. It also looks before this PR we are actually consistent (both Linux and macos use clocks that do not count when the system is asleep), and this PR makes us inconsistent by changing only Linux.

Or maybe I am looking at the wrong docs? This seems to look reasonably official though for macos.

RalfJung · 2022-11-30T07:53:35Z

library/std/src/sys/unix/time.rs

-            #[cfg(not(target_os = "macos"))]
+
+            #[cfg(target_os = "linux")]
+            const clock_id: libc::clockid_t = libc::CLOCK_BOOTTIME;


Should we have documentation saying that Instant, when possible, will use a clock that keeps going when the system is asleep -- and list on which targets this is definitely the case?

Judging from this and this PR, that would be Windows and Linux. No idea about Android, the BSDs, iOS/watchOS, ...

When I fixed it for my crate I looked into most platforms: https://github.com/LiveSplit/livesplit-core/blob/092e336e24abd26e97e5a2e55abb0bbd97ff0b44/src/platform/normal/mod.rs#L6

Formatted as Markdown: https://gist.github.com/CryZe/afb052b82839d36d5bdbe4e88551f4dc

Okay so it seems like after this PR it's pretty much "everything except macOS"?

Fuchsia is also problematic as they specifically defined their monotonic to behave like Linux's... but they didn't provide an alternative BOOTTIME one yet. Also the PR only checks for "linux" specifically and not Linux derivatives such as Android and l4re (not sure 100% on l4re, at least libc usually bundles it into the Linux-like #[cfg]s), which it probably should (just not emscripten as it errors out on BOOTTIME atm).

Okay, doesn't look like this actually closes #87906 quite yet then.

RalfJung · 2022-11-30T08:06:44Z

src/tools/miri/src/shims/time.rs

+                    // This is the equivalent to `CLOCK_UPTIME_RAW` in the macos section
+                    // We support it, because std relies on it


Suggested change

// This is the equivalent to `CLOCK_UPTIME_RAW` in the macos section

// We support it, because std relies on it

// Unlike `CLOCK_MONOTONIC` (which is weird on Linux in that it does not increment

// during system sleep), this *is* supposed to increment during system sleep...

// but that's not really something a program running inside Miri can tell, anyway.

// We support it because std relies on it.

m-ou-se · 2022-12-14T15:05:06Z

This issue was discussed again in the libs meeting last week. There is currently no consensus for switching to CLOCK_BOOTTIME.

One of the points that was brought up is that many Linux APIs like futex(), futex_waitv(), and pthread_cond_t only support CLOCK_REALTIME and CLOCK_MONOTONIC. In general, it seems that CLOCK_MONOTONIC is the 'default' monotonic clock on Linux in many situations, unlike CLOCK_BOOTTIME. It seems best for std's Instant to just use whatever is the 'standard' monotonic clock on each platform.

What was also mentioned is that without providing additional guarantees that hold across all platforms, switching just one platform doesn't seem that useful; users still wouldn't be able to rely on the exact behaviour.

hellow554 · 2022-12-15T09:28:44Z

So I'm gonna close this.
Feel free to return to this PR whenever we decide elsewise, but for now, it's okay to allow this PR to rest :)

rust-highfive assigned Mark-Simulacrum Sep 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021

hellow554 commented Sep 7, 2021

View reviewed changes

hellow554 force-pushed the boottime branch from 8be3ef4 to 290a69b Compare September 7, 2021 07:46

Mark-Simulacrum assigned joshtriplett and unassigned Mark-Simulacrum Sep 7, 2021

cuviper reviewed Sep 7, 2021

View reviewed changes

library/std/src/sys/unix/time.rs Outdated Show resolved Hide resolved

workingjubilee mentioned this pull request Sep 7, 2021

Instant::duration_since returns a shortened duration if a suspend occurred #87906

Open

joshtriplett reviewed Sep 7, 2021

View reviewed changes

library/std/src/sys/unix/time.rs Outdated Show resolved Hide resolved

the8472 added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-time Area: Time labels Sep 8, 2021

tobz mentioned this pull request Sep 23, 2021

Switch to CLOCK_BOOTTIME and friends to improve accuracy. metrics-rs/quanta#58

Closed

hellow554 force-pushed the boottime branch from 290a69b to 987456a Compare October 24, 2021 19:13

hellow554 force-pushed the boottime branch from 987456a to 2b40d0a Compare October 25, 2021 09:09

rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 25, 2022

schultetwin1 mentioned this pull request Sep 9, 2022

Platform differences on timers when OS is asleep cloudflare/boringtun#316

Closed

link2xt mentioned this pull request Sep 24, 2022

Timer is paused on Android in "doze" state smol-rs/async-io#63

Open

use CLOCK_BOOTTIME in Instant::now

3ab376d

There was a discussion on rust-lang#87907 that all platforms expect linux advance its monotonic timer if the system goes into a sleep state (e.g. suspend or hibernate). It was decided that CLOCK_BOOTTIME should be used on unix targets.

hellow554 force-pushed the boottime branch from 4200ea1 to 3ab376d Compare November 29, 2022 14:11

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2022

This comment has been minimized.

Sign in to view

add CLOCK_BOOTTIME to miri shims

2ab523d

RalfJung reviewed Nov 30, 2022

View reviewed changes

hellow554 closed this Dec 15, 2022

This was referenced Jul 9, 2023

Use clock BOOTTIME for thread::sleep on linux #113513

Closed

Tracking Issue for thread::sleep_until #113752

Open

iequidoo mentioned this pull request Dec 15, 2023

fix: Use SystemTime instead of Instant everywhere deltachat/deltachat-core-rust#5108

Merged

tmandry mentioned this pull request Nov 5, 2024

Use CLOCK_BOOTTIME for Instant in Fuchsia/Android #132331

Open

		// This is the equivalent to `CLOCK_UPTIME_RAW` in the macos section
		// We support it, because std relies on it

-                    // This is the equivalent to `CLOCK_UPTIME_RAW` in the macos section
-                    // We support it, because std relies on it
+                    // Unlike `CLOCK_MONOTONIC` (which is weird on Linux in that it does not increment
+                    // during system sleep), this *is* supposed to increment during system sleep...
+                    // but that's not really something a program running inside Miri can tell, anyway.
+                    // We support it because std relies on it.

use CLOCK_BOOTTIME in Instant::now #88714

use CLOCK_BOOTTIME in Instant::now #88714

Conversation

hellow554 commented Sep 7, 2021 • edited Loading

rust-highfive commented Sep 7, 2021

hellow554 Sep 7, 2021

Choose a reason for hiding this comment

workingjubilee Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

hellow554 Sep 7, 2021

Choose a reason for hiding this comment

joshtriplett Oct 4, 2021

Choose a reason for hiding this comment

CryZe commented Sep 7, 2021 • edited Loading

hellow554 commented Sep 7, 2021 • edited Loading

thomcc commented Sep 7, 2021

workingjubilee commented Sep 7, 2021 • edited Loading

joshtriplett commented Sep 7, 2021

jonhoo commented Sep 23, 2021

joshtriplett commented Oct 4, 2021

JohnCSimon commented Oct 19, 2021

hellow554 commented Oct 24, 2021

joshtriplett commented Oct 25, 2021

hellow554 commented Oct 25, 2021

maurer commented Sep 8, 2022

cuviper commented Sep 9, 2022

faern commented Nov 11, 2022

bors commented Nov 19, 2022

JohnCSimon commented Nov 27, 2022

mati865 commented Nov 27, 2022

rustbot commented Nov 29, 2022

hellow554 commented Nov 29, 2022

This comment has been minimized.

hellow554 commented Nov 29, 2022

maniwani commented Nov 29, 2022

rustbot commented Nov 30, 2022

RalfJung commented Nov 30, 2022 • edited Loading

RalfJung Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 30, 2022

Choose a reason for hiding this comment

RalfJung Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

CryZe Nov 30, 2022

Choose a reason for hiding this comment

RalfJung Nov 30, 2022

Choose a reason for hiding this comment

CryZe Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 30, 2022

Choose a reason for hiding this comment

RalfJung Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

m-ou-se commented Dec 14, 2022

hellow554 commented Dec 15, 2022

hellow554 commented Sep 7, 2021 •

edited

Loading

workingjubilee Sep 7, 2021 •

edited

Loading

CryZe commented Sep 7, 2021 •

edited

Loading

hellow554 commented Sep 7, 2021 •

edited

Loading

workingjubilee commented Sep 7, 2021 •

edited

Loading

RalfJung commented Nov 30, 2022 •

edited

Loading

RalfJung Nov 30, 2022 •

edited

Loading

RalfJung Nov 30, 2022 •

edited

Loading

CryZe Nov 30, 2022 •

edited

Loading

RalfJung Nov 30, 2022 •

edited

Loading