use CLOCK_BOOTTIME in Instant::now by hellow554 · Pull Request #88714 · rust-lan...
source link: https://github.com/rust-lang/rust/pull/88714
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
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.
(rust-highfive has picked a reviewer for you, use r? to override)
library/std/src/sys/unix/time.rs
Outdated
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.
library/std/src/sys/unix/time.rs
Outdated
Alright, so the situation is the following:
- All the resources I found seem to very roughly indicate that except for Linux
CLOCK_MONOTONICdoes 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_MONOTONICas 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.
macos currently uses mach_absolute_time (here) which states:
[...] this clock does not increment while the system is asleep.
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.
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.
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.
@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.
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.
Ping from triage:
@hellow554 Can you please post your status on this PR?
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?
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.
ALright, I wrapped it into if cfg! :)
Looks reasonable to me, assuming it passes CI.
@rfcbot merge
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
See this document for info about what commands tagged team members can give me.
Copy link
rfcbot commented 17 hours ago
This is now entering its final comment period, as per the review above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK