Switch Instant to use CLOCK_BOOTTIME #87907

maurer · 2021-08-10T04:19:03Z

For Linux-like platforms, use CLOCK_BOOTTIME which continues ticking
during suspend. Without this change, Duration between two Instants
can bear little relation to reality if a suspend took place in between.

Fixes #87906

For Linux-like platforms, use CLOCK_BOOTTIME which continues ticking during suspend. Without this change, `Duration` between two `Instant`s can bear little relation to reality if a suspend took place in between. Fixes rust-lang#87906

rust-highfive · 2021-08-10T04:19:06Z

r? @dtolnay

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

hellow554 · 2021-08-10T06:47:58Z

I would find it really difficult to only have this behaviour on linux.

What about windows? How does it currently do the time measurement? What about bsd, ios, redox, ... ?

I think if we touch one thing, we should update every OS.

maurer · 2021-08-11T07:06:48Z

I would find it really difficult to only have this behaviour on linux.

What about windows? How does it currently do the time measurement? What about bsd, ios, redox, ... ?

BSD, iOS, redox, and OSX would all be updated by this change as well - perhaps my commit message could have been worded better, but they all use unix/time.rs to define Instant, and so use these libc definitions.

Windows has difficulties because QueryPerformanceCounter which we use right now doesn't run through suspend, but GetTickCount (which runs through suspend) has lower resolution (possibly as low as 15ms, depending on system configuration). If any Windows experts know a better way to do this that'd be great, but it's harder to argue for updating Windows here since there would be a downside to the change the only way I know how to make it.

I think if we touch one thing, we should update every OS.

In an ideal world I agree, but I don't think that we want a lowest common denominator interface here. Just because Windows doesn't offer a high precision timer that ticks in suspend doesn't mean we shouldn't use it where supported.

maurer · 2021-08-11T07:19:56Z

My previous post was based on what is evidently old information. According to Microsoft they are now handling any necessary adjustments for sleep in QueryPerformanceCounter. As a result, this means that we are bringing the Unix-like implementation in line with the Windows one, and the two do not actually currently agree on semantics.

syvb · 2021-08-11T15:52:48Z

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

@@ -293,7 +293,7 @@ mod inner {

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


CLOCK_BOOTTIME isn't supported on Linux <= 2.6.39, but Rust supports Linux >= 2.6.32. This should check if -EINVAL is returned and fall back to CLOCK_MONOTONIC if so.

the8472 · 2021-08-11T16:24:43Z

BSD, iOS, redox, and OSX would all be updated by this change as well

osx has a separate implementation

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

Lines 149 to 155 in 362e0f5

    
           impl Instant { 
        
               pub fn now() -> Instant { 
        
                   extern "C" { 
        
                       fn mach_absolute_time() -> u64; 
        
                   } 
        
                   Instant { t: unsafe { mach_absolute_time() } } 
        
               }

workingjubilee · 2021-08-11T21:48:07Z

BSD, iOS, redox, and OSX would all be updated by this change as well - perhaps my commit message could have been worded better, but they all use unix/time.rs to define Instant, and so use these libc definitions.

This does not mean the code would remain correct. CLOCK_MONOTONIC is true on all these platforms because it is POSIX. This notably does not specify whether CLOCK_BOOTTIME is a sensical value on all Unix platforms. I expect this PR would cause other platforms to fail to build, like FreeBSD, which uses its own constants and not the "Linux-like" set in libc. As such, this PR must bifurcate the implementation of time.rs further in order to work and not impact tier 2 support promises.

syvb · 2021-08-11T21:50:36Z

@workingjubilee That would have to be done to support Tier 1 targets too, since older versions of Linux supported by Rust don't support CLOCK_BOOTTIME (#87907 (review))

maurer · 2021-08-12T03:30:31Z

BSD, iOS, redox, and OSX would all be updated by this change as well

osx has a separate implementation

Sorry, missed that unix/time.rs contained multiple implementations. It looks like we're using mach_absolute_time, which Apple says does not tick during suspend. They also have a function mach_continous_time which [does] tick during sleep. The discussion section for both calls recommend the use of clock_gettime_nsec_np with an appropriate clock ID over the use of those functions directly. clock_gettime and friends appears to have appeared in OSX 10.12, and our platform support says we support back to 10.7, so when I update this patch I'll switch it to use mach_continuous_time.

maurer · 2021-08-12T03:49:47Z

This does not mean the code would remain correct. CLOCK_MONOTONIC is true on all these platforms because it is POSIX. This notably does not specify whether CLOCK_BOOTTIME is a sensical value on all Unix platforms. I expect this PR would cause other platforms to fail to build, like FreeBSD, which uses its own constants and not the "Linux-like" set in libc. As such, this PR must bifurcate the implementation of time.rs further in order to work and not impact tier 2 support promises.

@workingjubilee
Since you seem to know more about FreeBSD than I do, do you have a suggestion for which clock we should use there? Reading through the man page and a bug where someone gets confused by the (admittedly documented) behavior of CLOCK_UPTIME, it sounds like there's currently no monotonically increasing clock which is also inclusive of suspend on FreeBSD. I'm tempted to just set them to CLOCK_MONOTONIC and add a comment in the documentation that some operating systems (insert a list there) don't support including sleep, and in those cases we just use a regular monotonic clock.

thomcc · 2021-08-12T06:13:58Z

On macOS 10.12 (iOS/tvOS 10, watchOS 3) clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW) can be used, which continues to increase across suspends, etc.

We'd have to dynamically load it (via weak! or similar) and fall back to the current code though, since we support much older versions of these targets (although it seems very likely that the majority of devices in the wild are likely to have this)

[1]: There's also mach_continuous_time which is equivalent, but it was added in/supports exactly the same versions, and is the official docs say to use clock_gettime_nsec_np(CLOCK_MONOTONIC_RAW) instead. That said, the API for mach_continuous_time is closer to mach_absolute_time, so it may be less of a hassle to conditionally swap out one for the other.

(That said, I would not really tempt Apple about using an API they explicitly tell you not to in the docs...)

the8472 · 2021-08-12T21:22:37Z

and add a comment in the documentation that some operating systems (insert a list there) don't support including sleep, and in those cases we just use a regular monotonic clock.

Imo that calls into question the value of this change. If we can't promise this behavior then someone may start to rely on non-portable implementation details because it worked on their machine but not on old (but supported) or less common platforms.

thomcc · 2021-08-12T21:30:35Z

I feel that way too, and am fairly torn on it.

But the original issue seems to imply people already rely on this, or at least failed to consider it. As a result, their their code is has some bugs in this case.

ISTM like it already behaves non-portably, but just in a weaker sense — it works fine on machines that don't get put to sleep much/ever (many servers, some desktop machines, etc), but this assumption is "not portable" to systems that do, like consumer phones, laptops, etc. (Yes, I'm aware this is an abuse of terminology)

As mentioned though, I'm torn on it, and could probably be convinced either way.

nbdd0121 · 2021-08-12T21:57:46Z

I think we should just document Instant to say that behaviour across suspend is not guaranteed.

CryZe · 2021-08-12T22:13:09Z

I'd say it absolutely should be documented that it can't guarantee that across all the platforms, such that people don't rely on it. But I'd still like to see this PR be merged. There's very little reason not to fix the behavior where possible. This otherwise just causes unnecessary bugs for lots of people for no good reason.

Let's say it this way: Yes, the documentation doesn't guarantee a lot about Instant other than that it's monotonic, but there's still an expectation of it being the de facto API to measure time. Therefore while yes, a monotonic implementation of Instant could just be an atomic integer that counts up by 1 on every call, or even always just return 0. That would absolutely be the fastest implementation and fits the docs, but that's just not a useful implementation. People want to use the type to measure time, so the best OS API that does that should be used, even if there's not a lot of specifics we can guarantee.

joshtriplett · 2021-08-18T19:24:35Z

Having a huge discontinuity over suspends isn't necessarily always the desirable behavior. If you're trying to do something like "update based on the amount of time that has passed", or "measure how long a computation took", you may want to skip suspended time.

I personally would like more of an ability to use other clocks. But I don't think we should change the behavior of now, particularly if we then get inconsistencies across platforms.

Perhaps we should have an InstantExt on UNIX with a method that allows passing an arbitrary libc::clockid_t and/or libc::timespec. That would let people choose whatever clock they want to use.

thomcc · 2021-08-18T19:26:45Z

Perhaps we should have an InstantExt on UNIX with a method that allows passing an arbitrary libc::clockid_t and/or libc::timespec. That would let people choose whatever clock they want to use.

This is problematic for Darwin, although I guess it could be used elsewhere.

joshtriplett · 2021-08-18T19:27:49Z

@thomcc Can you elaborate on why that would be problematic for Darwin?

thomcc · 2021-08-18T19:35:05Z

On darwin, the clock_gettime-alikes are only in Darwin 16 (macOS 10.12) and later. Before that you just have mach_absolute_time. This is recent enough for unchecked usage to be a bit of a problem.

Also, the versions that use timespecs are disrecommended in favor of the _nsec_np versions (which return direct nanoseconds), although I don't expect there's much likelihood that they'll remove the timespec-using versions, so this part probably doesn't matter.

joshtriplett · 2021-08-18T20:05:48Z

We discussed this in today's @rust-lang/libs-api meeting. We decided that we don't want to make this change for multiple reasons:

It isn't portable (and this PR doesn't address that)
It doesn't work on all versions of the Linux kernel that we still support.
This behavior is already inconsistent across platforms, and we can't make it consistently include suspend time like this.
We're not sure that this is the desired behavior, in any case, and the inconsistency seems worse.

Thus, we're going to close this issue.

However, we'd be interested in seeing work that makes Instant consistent in the other direction: if it's possible to ignore suspended time on all versions of Windows, we'd love to see a patch that switches to doing that. (cc @Amanieu who had some ideas about that in the meeting)

maurer · 2021-08-18T20:11:50Z

Having a huge discontinuity over suspends isn't necessarily always the desirable behavior. If you're trying to do something like "update based on the amount of time that has passed", or "measure how long a computation took", you may want to skip suspended time.

Part of my disagreement with this being useful is that this does not measure the amount of time passed (the system being suspended doesn't stop time), and doesn't measure how long a computation took (this clock keeps ticking even if your process gets SIGSTOP'd, or the kernel has other higher priority threads to evaluate). The only correct use I've seen for CLOCK_MONOTONIC that doesn't work with CLOCK_BOOTTIME is availability watchdogs.

I personally would like more of an ability to use other clocks. But I don't think we should change the behavior of now, particularly if we then get inconsistencies across platforms.

Perhaps we should have an InstantExt on UNIX with a method that allows passing an arbitrary libc::clockid_t and/or libc::timespec. That would let people choose whatever clock they want to use.

If we choose to go the route of "provide other clocks as things other than Instant", I don't think it should be through an OS-specific interface. I think it would make more sense to provide "A clock that ticks during suspend, and advances monotonically at a constant rate" "A clock which only runs while the machine is running" etc. as generic interfaces with OS-specific implementations when possible. the InstantExt route would require a user who wants a clock that ticks in suspend to write different code for Linux, Windows, and any UNIX-like that uses its own name for CLOCK_BOOTTIME in their application.

As an aside, if we did go the InstantExt route, I'd at least want the source clock included in the Instant to have dynamic checks to prevent accidental .duration_since() between different clocks.

joshtriplett · 2021-08-18T20:32:33Z

On Wed, Aug 18, 2021 at 01:12:01PM -0700, Matthew Maurer wrote: > Perhaps we should have an `InstantExt` on UNIX with a method that allows passing an arbitrary `libc::clockid_t` and/or `libc::timespec`. That would let people choose whatever clock they want to use. If we choose to go the route of "provide other clocks as things other than `Instant"`, I don't think it should be through an OS-specific interface. I think it would make more sense to provide "A clock that ticks during suspend, and advances monotonically at a constant rate" "A clock which only runs while the machine is running" etc. as generic interfaces with OS-specific implementations when possible. the `InstantExt` route would require a user who wants a clock that ticks in suspend to write different code for Linux, Windows, and any UNIX-like that uses its own name for `CLOCK_BOOTTIME` in their application. As an aside, if we did go the `InstantExt` route, I'd at least want the source clock included in the `Instant` to have dynamic checks to prevent accidental `.duration_since()` between different clocks.

I'm not even sure if InstantExt is the right interface for that; that was just one possibility. Some of the clocks available don't actually make sense as an Instant; for instance, some need the ability to access the raw value to make sense of them (e.g. CLOCK_PROCESS_CPUTIME_ID), so Instant as an opaque type doesn't work well there. But I'm also not sure we can provide a sufficiently portable implementation of "a clock that ticks during suspend", either.

maurer · 2021-08-18T20:35:19Z

But I'm also not sure we can provide a sufficiently portable implementation of "a clock that ticks during suspend", either.

Unless I've missed something, we can do that on Linux >= 2.6.39, OSX >= 10.12, and Windows (not sure of versions, I think the function we're using changes behavior if you go back far enough but I'm not certain), without much difficulty. That seems fairly portable...

joshtriplett · 2021-08-18T20:41:13Z

On Wed, Aug 18, 2021 at 01:35:30PM -0700, Matthew Maurer wrote: > But I'm also not sure we can provide a sufficiently portable implementation of "a clock that ticks during suspend", either. Unless I've missed something, we can do that on Linux > 2.6.39, OSX > 10.12, and Windows (not sure of versions, I think the function we're using changes behavior if you go back far enough but I'm not certain), without much difficulty. That seems fairly portable...

What about other UNIX systems? What should this implementation do on older Linux, older macOS, or older Windows, all of which we still officially support? I think it'd be appropriate for such an implementation to start in the crate ecosystem.

workingjubilee · 2021-08-18T20:43:20Z

Since you seem to know more about FreeBSD than I do, do you have a suggestion for which clock we should use there? Reading through the man page and a bug where someone gets confused by the (admittedly documented) behavior of CLOCK_UPTIME, it sounds like there's currently no monotonically increasing clock which is also inclusive of suspend on FreeBSD. I'm tempted to just set them to CLOCK_MONOTONIC and add a comment in the documentation that some operating systems (insert a list there) don't support including sleep, and in those cases we just use a regular monotonic clock.

@maurer: I am not aware of the best solution here, unfortunately. I am aware of what is POSIX and can smell a Linux-only extension at a thousand paces, and I believe that smaller platforms should be treated with appropriate respect, or at least, should successfully compile, so the introduction of Linux-only code that did not apply and would not compile, in a way that the full CI run would reject, was apparent to me in a way I could not help but note.

But I'm also not sure we can provide a sufficiently portable implementation of "a clock that ticks during suspend", either.

Unless I've missed something, we can do that on Linux >= 2.6.39, OSX >= 10.12, and Windows (not sure of versions, I think the function we're using changes behavior if you go back far enough but I'm not certain), without much difficulty. That seems fairly portable...

"fairly" is not "is", unfortunately.

Amanieu · 2021-08-19T00:29:16Z

Support for multiple clocks should be implemented using a generic Instant type over a Clock trait, like C++ does. It should not be hacked on to the existing Instant type.

Perhaps this is a good starting point for a third party crate which provides support for multiple clocks with different properties.

Amanieu · 2021-08-19T00:30:13Z

(cc @Amanieu who had some ideas about that in the meeting)

Actually I looked into it and it's only possible with some undocumented NT interfaces which aren't even stable across Windows versions. So that's probably right out.

CryZe · 2021-09-01T12:07:46Z

I've looked deeper into this and... this seems to literally be a Linux specific bug. On all other OSs CLOCK_MONOTONIC does the right thing on other Unixes including OpenBSD, macOS and iOS. And as this PR intended, CLOCK_BOOTTIME trivially resolves the bug on Linux. Also portability to old Linux kernels isn't a problem either, you just fall back to CLOCK_MONOTONIC if it errors (caching this so you can directly skip to CLOCK_MONOTONIC the next time). So with that said, I propose to reopen this PR (or I can open a fixed one).

Pretty sure none of these points apply anymore:

It isn't portable (and this PR doesn't address that)

It's portable via the fallback.

It doesn't work on all versions of the Linux kernel that we still support.

The fallback ensures it works on all kernels.

This behavior is already inconsistent across platforms, and we can't make it consistently include suspend time like this.

It's consistent then, atm it isn't.

We're not sure that this is the desired behavior, in any case, and the inconsistency seems worse.

Exactly, the inconsistency is worse, which is the status quo, so this making it more consistent is the better behavior.

workingjubilee · 2021-09-01T23:43:38Z

Thank you, CryZe, for doing the work there to verify the appropriate setting is indeed CLOCK_MONOTONIC on OpenBSD. Given that BSDs have some consistencies on these marks, it seems plausible to me that this is a correct assessment on all BSDs. I also believe that since FreeBSD has CLOCK_UPTIME as a POSIX extension, we can take it as "in contrast" to _MONOTONIC, though verification would be ideal.

I would like to note that on OpenBSD, however, the absolute value is still considered "meaningless" in a formal sense, promising only monotonic increase. It only "accidentally" aligns with a correct, monotonically increasing clock since boot time. So like Linux, they also introduced CLOCK_BOOTTIME, which is functionally identical to _MONOTONIC but has attached the formal promise that it updates across suspensions and is since boot, and that should probably be preferred on OpenBSD where possible. Consider https://man.openbsd.org/clock_gettime.2

But I think this only reinforces the point, however, that becoming consistent with "wall time" is most intuitive for the default Instant type, given that people keep "accidentally" selecting it. I believe it is also consistent with the goal of _MONOTONIC which is essentially to enable precise, nanosecond sleep on systems with clock_nanosleep, without having to care about possible interruptions.

joshtriplett · 2021-09-02T20:27:27Z

If changing the Linux behavior in this regard would indeed result in consistency across all targets, then I think it'd be a reasonable change. (I'd love to have the Linux behavior on all platforms, either by default or via some other call, but it sounds like that's much harder.)

However, I don't think we should implement caching behavior; that would optimize for the performance of repeated calls on ancient Linux at the expense of a slight hit on non-ancient Linux. I'd suggest, instead, that we do one of two things:

Always fall back from CLOCK_BOOTTIME to CLOCK_MONOTONIC on Linux if we get EINVAL. (Optionally omit the fallback code on Linux targets whose minimum kernel is already more than 2.6.39, which as far as I can tell means every non-x86 target.)
Consider bumping the minimum Linux kernel requirement from 2.6.32+ to 2.6.39+. 2.6.32 was released 2009-12-02, and 2.6.39 was released 2011-05-18. I think it's reasonable to require at least a ten-year-old kernel that's present not only in current enterprise distributions but in previous versions of enterprise distributions. Does anyone actually need support for kernels 2.6.32 to 2.6.38? cc @cuviper.

joshtriplett · 2021-09-03T04:01:43Z

@CryZe If you're interested in submitting a pull request implementing this, with an explanation in the code and commit message based on your research into the behavior of various operating systems, I'd be happy to review it.

cuviper · 2021-09-03T22:45:49Z

Does anyone actually need support for kernels 2.6.32 to 2.6.38?

I do, but only because our internal RHEL builders run on a kernel from the previous major release, so my RHEL 7 builds are running on RHEL 6's 2.6.32. So it's not something I need for customers, but it's important for me nonetheless.

I know it's a pain though, and I'm willing to shoulder the burden of writing/maintaining compatibility code in such cases. Just please notify me (as you did here) when there's something that needs such help.

joshtriplett · 2021-09-04T04:37:00Z

@cuviper Alright. In that case, I think we should just always fallback on EINVAL.

the8472 · 2021-09-06T22:59:58Z

2.6.39 would also unlock O_PATH which makes it easier to use the -at version of syscalls.

joshtriplett · 2021-09-07T00:13:01Z

@the8472 Based on further discussions with @cuviper, it sounds like upgrading the baseline version of the Linux kernel would require additional work and coordination. I'm hoping that may be possible at some point, but let's not block this proposed change on that, since it'd just take adding an unconditional fallback on EINVAL.

hellow554 · 2021-09-07T05:59:42Z

@rustbot claim

hellow554 · 2021-09-07T06:00:34Z

Wait a second, this isn't the issue, this is the PR... let's try that again there.

@rustbot release

Switch Instant to use CLOCK_BOOTTIME

e95612b

For Linux-like platforms, use CLOCK_BOOTTIME which continues ticking during suspend. Without this change, `Duration` between two `Instant`s can bear little relation to reality if a suspend took place in between. Fixes rust-lang#87906

rust-highfive assigned dtolnay Aug 10, 2021

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

syvb reviewed Aug 11, 2021

View reviewed changes

the8472 mentioned this pull request Aug 12, 2021

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

Open

dtolnay added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 14, 2021

joshtriplett closed this Aug 18, 2021

CryZe mentioned this pull request Aug 18, 2021

Fork Instant to measure time during suspension LiveSplit/livesplit-core#442

Closed

CryZe mentioned this pull request Aug 18, 2021

Instant is not suitable for measuring time LtPeriwinkle/mist#7

Closed

rustbot assigned hellow554 and unassigned dtolnay Sep 7, 2021

hellow554 mentioned this pull request Sep 7, 2021

use CLOCK_BOOTTIME in Instant::now #88714

Closed

Switch Instant to use CLOCK_BOOTTIME #87907

Switch Instant to use CLOCK_BOOTTIME #87907

Conversation

maurer commented Aug 10, 2021

rust-highfive commented Aug 10, 2021

hellow554 commented Aug 10, 2021

maurer commented Aug 11, 2021 • edited Loading

maurer commented Aug 11, 2021 • edited Loading

syvb Aug 11, 2021

Choose a reason for hiding this comment

the8472 commented Aug 11, 2021

workingjubilee commented Aug 11, 2021

syvb commented Aug 11, 2021

maurer commented Aug 12, 2021

maurer commented Aug 12, 2021

thomcc commented Aug 12, 2021

the8472 commented Aug 12, 2021

thomcc commented Aug 12, 2021

nbdd0121 commented Aug 12, 2021

CryZe commented Aug 12, 2021 • edited Loading

joshtriplett commented Aug 18, 2021

thomcc commented Aug 18, 2021

joshtriplett commented Aug 18, 2021

thomcc commented Aug 18, 2021

joshtriplett commented Aug 18, 2021

maurer commented Aug 18, 2021

joshtriplett commented Aug 18, 2021 via email

maurer commented Aug 18, 2021 • edited Loading

joshtriplett commented Aug 18, 2021 via email

workingjubilee commented Aug 18, 2021

Amanieu commented Aug 19, 2021

Amanieu commented Aug 19, 2021

CryZe commented Sep 1, 2021 • edited Loading

workingjubilee commented Sep 1, 2021 • edited Loading

joshtriplett commented Sep 2, 2021

joshtriplett commented Sep 3, 2021

cuviper commented Sep 3, 2021

joshtriplett commented Sep 4, 2021

the8472 commented Sep 6, 2021

joshtriplett commented Sep 7, 2021

hellow554 commented Sep 7, 2021

hellow554 commented Sep 7, 2021

maurer commented Aug 11, 2021 •

edited

Loading

maurer commented Aug 11, 2021 •

edited

Loading

CryZe commented Aug 12, 2021 •

edited

Loading

maurer commented Aug 18, 2021 •

edited

Loading

CryZe commented Sep 1, 2021 •

edited

Loading

workingjubilee commented Sep 1, 2021 •

edited

Loading