fix potential race in AtomicU64 time monotonizer #89017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
the8472
added
the
T-libs
AGSaidi
reviewed
Sep 16, 2021
kennytm
reviewed
Sep 17, 2021
|
@bors r+ |
|
📌 Commit 57465d9 has been approved by |
bors
added
S-waiting-on-bors
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Sep 19, 2021
…=kennytm fix potential race in AtomicU64 time monotonizer The AtomicU64-based monotonizer introduced in rust-lang#83093 is incorrect because several threads could try to update the value concurrently and a thread which doesn't have the newest value among all the updates could win. That bug probably has little real world impact since it doesn't make observed time worse than hardware clocks. The worst case would probably be a thread which has a clock that is behind by several cycles observing several inconsistent fixups, which should be similar to observing the unfiltered backslide in the first place. New benchmarks, they don't look as good as the original PR but still an improvement compared to the mutex. I don't know why the contended mutex case is faster now than in the previous benchmarks. ``` actually_monotonic() == true: test time::tests::instant_contention_01_threads ... bench: 44 ns/iter (+/- 0) test time::tests::instant_contention_02_threads ... bench: 45 ns/iter (+/- 0) test time::tests::instant_contention_04_threads ... bench: 45 ns/iter (+/- 0) test time::tests::instant_contention_08_threads ... bench: 45 ns/iter (+/- 0) test time::tests::instant_contention_16_threads ... bench: 46 ns/iter (+/- 0) atomic u64: test time::tests::instant_contention_01_threads ... bench: 66 ns/iter (+/- 0) test time::tests::instant_contention_02_threads ... bench: 287 ns/iter (+/- 14) test time::tests::instant_contention_04_threads ... bench: 296 ns/iter (+/- 43) test time::tests::instant_contention_08_threads ... bench: 604 ns/iter (+/- 163) test time::tests::instant_contention_16_threads ... bench: 1,147 ns/iter (+/- 29) mutex: test time::tests::instant_contention_01_threads ... bench: 78 ns/iter (+/- 0) test time::tests::instant_contention_02_threads ... bench: 652 ns/iter (+/- 275) test time::tests::instant_contention_04_threads ... bench: 900 ns/iter (+/- 32) test time::tests::instant_contention_08_threads ... bench: 1,927 ns/iter (+/- 62) test time::tests::instant_contention_16_threads ... bench: 3,748 ns/iter (+/- 146) ```
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 19, 2021
Rollup of 10 pull requests Successful merges: - rust-lang#87960 (Suggest replacing an inexisting field for an unmentioned field) - rust-lang#88855 (Allow simd_shuffle to accept vectors of any length) - rust-lang#88966 (Check for shadowing issues involving block labels) - rust-lang#88996 (Fix linting when trailing macro expands to a trailing semi) - rust-lang#89017 (fix potential race in AtomicU64 time monotonizer) - rust-lang#89021 (Add a separate error for `dyn Trait` in `const fn`) - rust-lang#89051 (Add intra-doc links and small changes to `std::os` to be more consistent) - rust-lang#89053 (refactor: VecDeques IntoIter fields to private) - rust-lang#89055 (Suggest better place to add call parentheses for method expressions wrapped in parentheses) - rust-lang#89081 (Fix a typo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The AtomicU64-based monotonizer introduced in #83093 is incorrect because several threads could try to update the value concurrently and a thread which doesn't have the newest value among all the updates could win.
That bug probably has little real world impact since it doesn't make observed time worse than hardware clocks. The worst case would probably be a thread which has a clock that is behind by several cycles observing several inconsistent fixups, which should be similar to observing the unfiltered backslide in the first place.
New benchmarks, they don't look as good as the original PR but still an improvement compared to the mutex.
I don't know why the contended mutex case is faster now than in the previous benchmarks.