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

[the8472]

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

[rust-highfive]

r? @Mark-Simulacrum

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

[jhpratt]

If I have instant2.duration_since(instant1) == Duration::from_secs(0), logic would say that instant1 == instant2. If the operation saturates, this invariant would no longer be upheld. Making elapsed saturate instead of panic has similar issues as you can mutate an Instant (via add-assigning a Duration).

As mentioned,

non-monotonic intervals will not be associative, i.e. b.duration_since(a) + c.duration_since(b) != c.duration_since(a)

I don't think anyone would expect that the two sides would be unequal in any situation. duration_since realistically only exists because implementing Sub would be a footgun. I fear that this may be on the same level.

---

While I understand the motivation, I don't think breaking logical invariants (even if not explicitly stated) is a reasonable solution.

[joshtriplett]

I love the idea of making this change, and removing all the associated complexity and panics.

I think in the common case people won't get two instants backwards, because they can call start.elapsed() rather than calling Instant::now() again. I can imagine cases motivating a second call to Instant::now() (e.g. making sure several durations are back-to-back by using the end of one interval as the start of the next), but it still seems acceptable to not catch reversing the two. It might also be possible to implement a simple lint for common cases.

I don't think this would affect any invariants people rely on in practice. And people can always call checked_duration_since(...).expect(...) if they want a panic.

[nagisa]

Is there any reason why we might want to keep actually_monotonic in places and continue using a simple wrapping subtraction instead of a saturating one on those platforms?

[the8472]

Currently there is no wrapping subtraction available on the sys::time::Instant implementations because on many platforms it's a composite type consisting of separate seconds and nanos fields. Only checked add/sub are provided.
Only the solid/itron target uses an Instant that's a single u64 field.

[bors]

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

[BurntSushi]

Initially, my main concern was that this silences potentially important failure modes. Namely, I can imagine someone relying on the fact that Instant::duration_since will panic if the underlying clock is not actually monotonic. But it looks like we're already trying to paper over that with a work-around that would silence such a failure mode anyway. So I guess that's not a new issue introduced by this change?

Changing algebraic laws also doesn't feel that great to me either. Technically they aren't documented, although a reasonable person might infer them from the panicking conditions of duration_since.

For those that understand this issue better than me: what is the cost of not doing this?

[the8472]

For those that understand this issue better than me: what is the cost of not doing this?

• sporadic panics on systems marked as actually_monotonic() == true
• contention and overhead due to the monotonizing mechanism on systems marked actually_monotonic() == false, for something that should be a very-low-overhead, contention-free clock source on modern systems.
• implementation complexity
• ongoing maintenance adjusting actually_monotonic() as affected systems get retired or new ones are added

[BurntSushi]

* spurious panics on systems marked as `actually_monotonic() == true`

I think this is the one that interests me to the most. Although I am perhaps wrong to find this one interesting. In particular---while again acknowledging that I don't work in such a domain---I would imagine that such a panic would be highly interesting to someone that wants a strong guarantee about monotonicity. That is, they might have written their code in a way where they want to see a panic if the underlying clock source didn't produce monotonic values. Maybe they should have used checked_duration_since for this, but duration_since is currently documented to panic in such cases, so they might have reasoned that it was acceptable to use in such a circumstance. AIUI, this patch will translate that from an expected explicit failure scenario to an unexpected silent failure.

With that said, I am pretty much reasoning in the abstract here. I don't have enough experience to put my feet on the ground here. The silent failure mode does concern me, but maybe my abstract reasoning is too much of a stretch.

If we have folks working with the guarantees of monotonic clocks in production that we could ping, getting their input on a change like this would probably be wise.

[the8472]

As discussed in the libs-api meeting I have changed the methods to panic in debug mode (via overflow-checks) and saturate in release mode.

Discussion on exactly which of {duration_since, elapsed, sub} should saturate or panic is still open.

[matklad]

I think we hit "time goes backwards in a VM" bug in NEAR: https://near.zulipchat.com/#narrow/stream/295302-dev-general/topic/general/near/248247028. So, for us, such a change would be very much welcome.

However, there's a wrinkle -- we compile our code in --release mode, but with overflow checks on. That is:

• We do want to panic due to "logical errors" with arithmetics
• If the underlying hardware/hypervisor/OS breaks monotonicity guarantees, we don't want to panic

For --release,overflow-checks = true, I fear that this specific implementation would be problematic -- it'll lead to strictly more panics than we have today (as we remove is_actually_monotonic check).

Ideally, I think we shouldn't be piggybacking on overflow-checks here, but instead used cfg!(debug_assertions). But of course it's not something we can do in std.

To sum up:

• I really like "panic in debug, saturate in release" approach, as it improves both reliability and performance
• The specific overflow_checks approach seems to make --release,overflow-checks = true more panicky, and that feels like a major regression.
• don't know how to what alternative impl approach would be better here though.

[the8472]

I think we hit "time goes backwards in a VM" bug in NEAR

If you're already hitting those panics that should mean that actually_monotonic() == false on your platform and the hardware clock is getting passed through unfiltered. So this change wouldn't make things worse for you, you just wouldn't get the intended benefits.

Also, that link would require signup to read it.

---

Depending on how much control you have over where the Instant's are compared maybe you could selectively change the overflow-checks flag for those crates via profile overrides?

[matklad]

Yeah, practically for near specifically I think we currently only run on is_actually_monotonic platforms. But in general any code which is compiled with release,overflow-checks=true mode and is run on !is_actually_monotonic platform will see more panics. Can't point to specific projects which use this scenario, but I'd be surprised if there aren't any.

Also, that link would require signup to read it.

Oups, good catch! That's the usual Zulip's "login with GitHub" thing, we should set-up zulip archive. The short discussion here basically just links to tokio-rs/tokio#3619

[bors]

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

[bors]

📌 Commit 37a1fc5 has been approved by Mark-Simulacrum

[uazu]

Seeing this on Rust Weekly News gave me a shock. So Instant::now() is no longer guaranteed non-decreasing? I think people need a heads-up about this. I think there are cases where this could break people's assumptions about ordering of operations, for example if I set two timers at one second from now, setting them from times 1ms apart, they could execute in the opposite order. This could break some assumptions the coder is relying on, for example if both of these timers update the same state, with the assumption that the second timer will execute second, then the state could be left with an intermediate value.

I just checked my code, and I don't use Instant::now() directly, but I keep a local copy which already has checks so that it can't go backwards (i.e. I already don't trust Instant::now()), so my code is okay. I wonder whether it's possible to judge whether this will break things for anyone else. I am in favour of the change, because getting rid of that overhead is an advantage. But if it is a breaking change for anyone, they need to know.

I'm trying to think of the cases where this could break. Maybe when two ordered events come in from an external source, e.g. from hardware or over a network stream, and two timers are set from those events, and then the timers execute in the opposite order, breaking the coder's assumptions. For this to have worked in the case of the previous Instant implementation, the timer queue would need to maintain the order of timers submitted for the same Instant, but if it did then this would be a breaking change for that coder. If this sounds unlikely to happen in real life code, then perhaps everything is okay.

Edit: Really it comes down to whether there are scenarios that would have worked with the previous implementation that won't work now. Thinking about it, in practice the time getting stuck on the same value for Z ms can also give unexpected problems.

[the8472]

So Instant is no longer guaranteed non-decreasing?

From the updated docs

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.

From an API perspective Rust already relied on the OS guarantees to provide monotonic time. It additionally applies some workarounds that are only relevant in situations when operating system fails to uphold its contract. What changed are the workarounds. What doesn't change is that any violation of monotonicity already is a platform bug, i.e. running rust on a broken system. This is akin to, albeit less severe, a filesystem not upholding its durability guarantees for example.

[uazu]

From an API perspective Rust already relied on the OS guarantees to provide monotonic time. It additionally applies some workarounds that are only relevant in situations when operating system fails to uphold its contract. What changed are the workarounds. What doesn't change is that any violation of monotonicity already is a platform bug, i.e. running rust on a broken system. This is akin to, albeit less severe, a filesystem not upholding its durability guarantees for example.

I understand that. However if a coder requires a strictly non-decreasing Instant (e.g. for their code to operate correctly), previously they could have relied on Rust's implementation to do the work-around in case of platform bugs. Now they need to do the work-around themselves. Maybe that's a reasonable thing to ask them to do, but at least they'll need a heads-up so that they can check their code (as I did), e.g. a warning in the Rust release notes or something.

[cuviper]

This is labeled relnotes already, but please feel free to check on that when we're drafting 1.60.0.

[nyetwurk]

IMO what is missing from this thread is any mention of pressuring vendors that have known broken VM/hypervisors in production (AWS). Hiding this is enabling their continued bad behavior.

[the8472]

However if a coder requires a strictly non-decreasing Instant (e.g. for their code to operate correctly), previously they could have relied on Rust's implementation to do the work-around in case of platform bugs. Now they need to do the work-around themselves.

If it is a strict requirement then using a system that isn't broken should be preferable. As it is preferable to put some critical transactional database on storage which honors FLUSH CACHE commands rather than putting a on a drive that YOLOs things (and thus isn't spec-compliant).

IMO what is missing from this thread is any mention of pressuring vendors that have known broken VM/hypervisors in production (AWS). Hiding this is enabling their continued bad behavior.

Well, users did encounter panics and filed bug reports with Rust but not with AWS. Or if they did they went unheard I guess.
"Panics will continue until compliance improves" is a possible strategy but not one that fits Rust's MO.

If there's an interest in stress-testing clocks on new hardware this could be done with a standalone program, similar to memory or GPU stress testing programs. It doesn't have to happen in production software.