r? @joshtriplett

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

@rustbot label T-libs

I think the general approach is very reasonable.

As funny as I find "twice_a_day", I think this should use more mundane naming. I also think it needs RUST_ in the name.

I would suggest RUST_CLOCK_ASSUME_MONOTONIC=1 and RUST_CLOCK_ASSUME_MONOTONIC=0, with unset meaning the default automatic behavior.

Sure, done.

Are environment controls considered part of the standard library's stable API?

@the8472 To answer your other question:

should this be documented or remain an undocumented bandaid we can offer to users if they run into reliability/performance problems?

I think we should absolutely document this. I also think we should very explicitly say "if you need to use RUST_CLOCK_ASSUME_MONOTONIC=0, please let us know so we can improve things to work more automatically in the future".

Ok. We still don't have stability markers for module documentation, so I'll add some "subject to change" note to the prose so it shouldn't be considered insta-stable.

I think the environment variable should perhaps be an opt-in to the "maybe buggy" behavior rather than the reverse. Otherwise I would expect it to be common advice to set it - most code won't notice the performance hit of the mutex, but would likely notice the possibility of panics and want to avoid that.

I think I'd feel differently if we expected this to be limited to non-tier-1 targets or similar, but from what I can tell this is likely to indefinitely be a problem for us without a - somewhat costly - solution. Perhaps that cost can be pushed up into the kernel, I'm not sure; unless we know that the function we're calling to get the timestamp is backed by a similar mutex (whether in kernel or other host environment) it seems like we should just make sure of correctness ourselves.

Some of that was already discussed in https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/more.20broken.20clocks

Transplanting my concerns/comments from Zulip:

I think that people who deploy their software to any broken hardware will be inclined to add something like env::set_var inside their code. I know I would do, because to me my code working across all hardware without having to worry about environment detail is way more important than the throughput of the timing function call. While setting the environment variable from code is likely to work, the caching behaviour is also prone to be a major stability hazard. In particular, adding calls to read time anywhere in code would become a potentially breaking change. Any code of the form:

fn main() {
    didnt_check_time_before() // but now does
    env::set_var("RUST_CLOCK_ASSUME_MONOTONIC", "0");
    Instant::now() // this is supposed to set up the cache.
}

would observe a change in behaviour once didnt_check_time_before starts calling the time functions. This extends to the point where we'd have to stay mindful of this caveat during initialization before main is called. It could also be something as innocuous as a log statement.

EDIT I doubt that saying "this is meant for debugging and is not covered by stability guarantees" will prevent anybody from relying on this environment variable to make their code work on the sandybridges they may come across, to be honest.

Do we have any data on how big of a performance impact the current workaround is?

I think the environment variable should perhaps be an opt-in to the "maybe buggy" behavior rather than the reverse. Otherwise I would expect it to be common advice to set it - most code won't notice the performance hit of the mutex, but would likely notice the possibility of panics and want to avoid that.

Down that road lie bad equilibria.

• it extinguishes any signal we have, on all platforms, about the quality of clocks, making it much more difficult in the future to revert that policy
• it duplicates efforts on other levels of the system (kernel, hardware, other languages)
• it imposes a toll on every system to protect against a few broken ones, even as hardware improves
• it leads to latency spikes (thread holding mutex getting preempted) on what should be a constant-time, low-overhead, contention-free operation

The current approach is a one-way ratchet where we mark some platform as non-monotonic when a user reports an issue and then never undo that because there is no mechanism to gauge the opposite case.

Ideally we would make actually_monotonic() { true } on nightly builds so people would give us more feedback, rather than fewer.

I think I'd feel differently if we expected this to be limited to non-tier-1 targets or similar, but from what I can tell this is likely to indefinitely be a problem for us without a - somewhat costly - solution. Perhaps that cost can be pushed up into the kernel, I'm not sure; unless we know that the function we're calling to get the timestamp is backed by a similar mutex (whether in kernel or other host environment) it seems like we should just make sure of correctness ourselves.

Yes, the API contract of CLOCK_MONOTONIC is that it is monotonic. Windows makes similar claims and yet we treat windows as never monotonic. So we should treat this as an OS or hardware bug instead.

The issue is easy to paper over, and we should have an option to do that. But making it visible should also be an option, we shouldn't guarantee to hide platform bugs forever.

I think that people who deploy their software to any broken hardware will be inclined to add something like env::set_var inside their code. I know I would do, because to me my code working across all hardware without having to worry about environment detail is way more important than the throughput of the timing function call. While setting the environment variable from code is likely to work, the caching behaviour is also prone to be a major stability hazard. In particular, adding calls to read time anywhere in code would become a potentially breaking change. Any code of the form:

fn main() {
    didnt_check_time_before() // but now does
    env::set_var("RUST_CLOCK_ASSUME_MONOTONIC", "0");
    Instant::now() // this is supposed to set up the cache.
}

The problem is with the environment though, so the intent is to give people the option to mark specific environments as unreliable, not to make it part of the code.

EDIT I doubt that saying "this is meant for debugging and is not covered by stability guarantees" will prevent anybody from relying on this environment variable to make their code work on the sandybridges they may come across, to be honest.

Do we have anything like #[cfg(channel = "nightly")]? Stability annotations aren't applicable here since this isn't an API that we want to hide but behavior we would have to toggle.

I don't want to use unstable function because that makes it more difficult for users of cargo installed binaries.

I could also remove the documentation part referring to the environment variable and only leave the request to report bugs and we can then direct users to the variable when they have filed a bug.

Do we have any data on how big of a performance impact the current workaround is?

That depends on whether you're interested in throughput or worst case latency. It also depends on how many threads you have and how frequently they call Instant::now().

rdtsc itself is perfectly scalable, it is always executes with about the same instruction throughput no matter how many cores are using it. The workaround on the other hand is the opposite of scalable, it is serializing.
It uses a Mutex, which means if the one thread is in the critical section and gets preempted then another thread can get stalled for however long it takes the scheduler to wake up one thread, exit the critical section and then wake up the other thread. On the uncontended case the mutex never leaves userspace and only requires 2 uncontended atomic atomic operations to do its job.

The kernel can do a better workaround than perfectly serializing because rdtsc actually returns a 64bit value - instead of the 128bit timespec - which can be monotonized with a load-max-cmpxchg sequence which can bail out early which allows multiple threads to make progress while anything working with a mutex or 128bit values only allows 1 thread at a time to make progress.

My optimization in #83093 does not change the general picture for unix systems, it only reduces the constant overhead, not the asymptotics. On windows we can do better since it exposes a 64bit counter value.

The problem is with the environment though, so the intent is to give people the option to mark specific environments as unreliable, not to make it part of the code.

Addressing this particular comment now because I recently heard a relevant analogy (VMs are cattle, not pets) from my coworker with regards to cloud instances. Any organization dealing with more than a couple of instances in the cloud will want to ensure instances have no code specific to address particularities of the specific instances their applications run on. For that reason all sorts of details end up in the application code instead. This environment variable would very much too, most likely unconditionally.

I can see the same rationale applying to the applications that run on the user machines.

Do we have anything like #[cfg(channel = "nightly")]?

I don't believe so.

The problem is with the environment though, so the intent is to give people the option to mark specific environments as unreliable, not to make it part of the code.

Addressing this particular comment now because I recently heard a relevant analogy (VMs are cattle, not pets) from my coworker with regards to cloud instances. Any organization dealing with more than a couple of instances in the cloud will want to ensure instances have no code specific to address particularities of the specific instances their applications run on. For that reason all sorts of details end up in the application code instead. This environment variable would very much too, most likely unconditionally.

I do understand the desire, but I do not want to actively support it because it leads to bad places as already outlined in my previous comment. It's similar to protocol ossification or permanent technical debt that nobody wants to clean up.
It's the result of seeking an easy short-term fix without considering the long term. This is not a fix, but a bandaid and a means to relax the default to allow more systems to benefit from fast timers because those who have broken systems can opt into the workaround. Bandaids have to come off at some point, as people replace their hardware or update firmware or hypervisors. That's why it shouldn't be baked into code.

If anything this makes me think we need a less stable mechanism to make this available. Maybe call it RUST_1_53_CLOCK_ASSUME_MONOTONIC derive the string from the compiler version or edition or something? Then people have to reapply it occasionally, which provides incentives to get things fixed.

Since controlling this via environment variables seems contentious, here's another approach:

1. change Instant::{elapsed, duration_since} to saturating
2. deprecate Instant::saturating_duration_since
3. remove all the backslide protection code

Instead of panics backslides would now saturate to zero. The downside is that duration_since would no longer catch the programming error where the start and end time of an interval are swapped, the developer either has to sanity-check the output or switch to checked_duration_since to catch it.
Another downside would be that backslides become observable via checked_duration_since, especially on systems that don't have a clock that guarantees monotonicity.

I'm in favor of saturating (#88652 (comment)).

I would be in favor of saturating by default as well.

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

Closing in favor of #89926