Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase Duration approximate equal threshold to 1us #56059

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

alexcrichton
Copy link
Member

Previously this threshold when testing was 100ns, but the Windows
documentation states:

which is a high resolution (<1us) time stamp

which presumably means that we could have up to 1us resolution, which
means that 100ns doesn't capture "equivalent" time intervals due to
various bits of rounding here and there.

It's hoped that this..

Closes #56034

Previously this threshold when testing was 100ns, but the Windows
documentation states:

> which is a high resolution (<1us) time stamp

which presumably means that we could have up to 1us resolution, which
means that 100ns doesn't capture "equivalent" time intervals due to
various bits of rounding here and there.

It's hoped that this..

Closes rust-lang#56034
@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2018
@alexcrichton
Copy link
Member Author

cc @kennytm

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2018

📌 Commit 8607325 has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 19, 2018
kennytm added a commit to pietroalbini/rust that referenced this pull request Nov 19, 2018
Increase `Duration` approximate equal threshold to 1us

Previously this threshold when testing was 100ns, but the Windows
documentation states:

> which is a high resolution (<1us) time stamp

which presumably means that we could have up to 1us resolution, which
means that 100ns doesn't capture "equivalent" time intervals due to
various bits of rounding here and there.

It's hoped that this..

Closes rust-lang#56034
bors added a commit that referenced this pull request Nov 19, 2018
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor useful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)
 - #56059 (Increase `Duration` approximate equal threshold to 1us)
@bors bors merged commit 8607325 into rust-lang:master Nov 19, 2018
@alexcrichton alexcrichton added beta-nominated Nominated for backporting to the compiler in the beta channel. beta-accepted Accepted for backporting to the compiler in the beta channel. labels Nov 19, 2018
@alexcrichton alexcrichton added this to the Rust 2018 Release milestone Nov 19, 2018
@alexcrichton alexcrichton deleted the fix-tests branch November 19, 2018 22:19
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 19, 2018
@nikomatsakis nikomatsakis mentioned this pull request Nov 20, 2018
20 tasks
bors added a commit that referenced this pull request Nov 20, 2018
beta backport rollup

Backports of some beta-approved PRs

- [x] #55385: NLL: cast causes failure to promote to static
- [x] #56043: remove "approx env bounds" if we already know from trait
- [x] #56003: do not propagate inferred bounds on trait objects if they involve `Self`
- [x] #55852: Rewrite `...` as `..=` as a `MachineApplicable` 2018 idiom lint
- [x] #55804: rustdoc: don't inline `pub use some_crate` unless directly asked to
- [x] #56059: Increase `Duration` approximate equal threshold to 1us
- [x]  Keep resolved defs in path prefixes and emit them in save-analysis #54145
- [x]  Adjust Ids of path segments in visibility modifiers #55487
- [x]  save-analysis: bug fix and optimisation. #55521
- [x]   save-analysis: be even more aggressive about ignorning macro-generated defs #55936
- [x]  save-analysis: fallback to using path id #56060
- [x]  save-analysis: Don't panic for macro-generated use globs #55879
- [x]  Add temporary renames to manifests for rustfmt/clippy #56081
- [x] Revert #51601 #56049
- [x]  Fix stability hole with `static _` #55983
- [x] #56077
- [x] Fix Rustdoc ICE when checking blanket impls #55258
- [x]  Updated RELEASES.md for 1.31.0 #55678
- [x] ~~#56061~~ #56111
- [x]  Stabilize `extern_crate_item_prelude` #56032

Still running tests locally, and I plan to backport @nrc's other PRs too

(cc @petrochenkov -- thanks for the advice)
Centril added a commit to Centril/rust that referenced this pull request Jan 24, 2019
Fix Instant/Duration math precision & associativity on Windows

**tl;dr** Addition and subtraction on Duration/Instant are not associative on windows because we use the perfcounter frequency in every calculation instead of just when we measure time.

This is my first contrib (PR or Issue) to Rust, so please lmk if I've done this wrong. I followed CONTRIBUTING to the extent I could given my system doesn't seem to be able to build the compiler with changes in the source tree. I also asked about this issue in #rust-beginners a week or so ago, before digging through libstd -- I'm unsure if there's a good way to follow up on that, but I'd be happy to update the docs on the timing structs if this fixes the problem.

## Issue

The `Duration` type keeps seconds in the upper-64 and nanoseconds in the lower-32 bits. In theory doing math on these ought to be basically the same as doing math on any other 64 or 32 bit integral number.

On windows (and I think macos too), however, our math gets messy because the Instant type stores the current point in time in units of HPET Performance Counter counts, not nanoseconds, and does unit conversions on every math operation, rather than just when we measure the time from the system clock.

I tried this code:

```
use std::time::{Duration, Instant};

fn main() {
    let now = Instant::now();
    let offset = Duration::from_millis(5);
    assert_eq!((now + offset) - now, (now - now) + offset);
}
```

On UNIX machines (linux and macos) it behaves as you'd expect -- [no crash](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=cf2206c0b7e07d8ecc7767a512364094).

On Windows hosts, however, it blows up because of a precision problem in the Instant +/- Duration math:

```
C:\Users\aberg\work\timetest (master -> origin)
λ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.02s
     Running `target\debug\timetest.exe`
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `4.999914ms`,
 right: `5ms`', src\main.rs:6:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: process didn't exit successfully: `target\debug\timetest.exe` (exit code: 101)

C:\Users\aberg\work\timetest (master -> origin)
λ cat src\main.rs
use std::time::{Duration, Instant};

fn main() {
    let now = Instant::now();
    let offset = Duration::from_millis(5);
    assert_eq!((now + offset) - now, (now - now) + offset);
}
```

On windows I think this is a consequence of doing the HPET-PerfCounter-Unit conversion on each math operation. I suspect the reason it works on macs is that (from what I could find online) most apple machines report timing in nanoseconds anyway. For anyone interested, the equivalent functions on macos, with a little work to fish out the numerator/denominator from a timebase struct:

* `QueryPerformanceCounter()` -> `mach_absolute_time()`
* `QueryPerformanceFrequency()` -> `mach_timebase_info()`

If this PR ends up working as I expect it to when CI runs the tests, I can make the same changes to the macos implementation.

## Potential Fix

We ought to be able to sort this out by storing nanoseconds, rather than PerfCounter units, that way intermediate math is done in the most precise units we support and we're only doing unit conversions when we actually measure the system clock (and it might even translate to a small perf gain for people doing tons of Instant/Duration math).

I believe this will address the underlying cause of rust-lang#56034, and make the guessed epsilon constant from rust-lang#56059 unnecessary. If it's of interest, I can write up how these timing types work on the tier 1 platforms to address rust-lang#32626 as well, since I'm already in here figuring it out.

## This Patch

To that end, I've got this patch, which I think should fix it on windows, but I'm having trouble testing it -- any time I change anything in libstd I start getting this error, which no amount of clean building seems to resolve:

```
C:\Users\aberg\work\rust (master -> origin)
λ python x.py test --stage 0 --no-doc src/libstd
Updating only changed submodules
Submodules updated in 0.27 seconds
    Finished dev [unoptimized] target(s) in 2.41s
Building stage0 std artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
    Finished release [optimized] target(s) in 6.78s
Copying stage0 std from stage0 (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc / x86_64-pc-windows-msvc)
Building stage0 test artifacts (x86_64-pc-windows-msvc -> x86_64-pc-windows-msvc)
   Compiling test v0.0.0 (C:\Users\aberg\work\rust\src\libtest)
error[E0460]: found possibly newer version of crate `std` which `getopts` depends on
  --> src\libtest\lib.rs:36:1
   |
36 | extern crate getopts;
   | ^^^^^^^^^^^^^^^^^^^^^
   |
   = note: perhaps that crate needs to be recompiled?
   = note: the following crate versions were found:
           crate `std`: \\?\C:\Users\aberg\work\rust\build\x86_64-pc-windows-msvc\stage0-sysroot\lib\rustlib\x86_64-pc-windows-msvc\lib\libstd-d7a80ca2ae113c97.rlib
           crate `std`: \\?\C:\Users\aberg\work\rust\build\x86_64-pc-windows-msvc\stage0-sysroot\lib\rustlib\x86_64-pc-windows-msvc\lib\std-d7a80ca2ae113c97.dll
           crate `getopts`: \\?\C:\Users\aberg\work\rust\build\x86_64-pc-windows-msvc\stage0-test\x86_64-pc-windows-msvc\release\deps\libgetopts-ae40a96de5f5d144.rlib

error: aborting due to previous error

For more information about this error, try `rustc --explain E0460`.
error: Could not compile `test`.

To learn more, run the command again with --verbose.
command did not execute successfully: "C:\\Users\\aberg\\work\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "build" "--target" "x86_64-pc-windows-msvc" "-j" "12" "--release" "--manifest-path" "C:\\Users\\aberg\\work\\rust\\src/libtest/Cargo.toml" "--message-format" "json"
expected success, got: exit code: 101
failed to run: C:\Users\aberg\work\rust\build\bootstrap\debug\bootstrap test --stage 0 --no-doc src/libstd
Build completed unsuccessfully in 0:00:20
```

---

Since you wrote the linked PRs and might remember looking at related problems:

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants