Waker::will_wake: Compare vtable address instead of its content #119863
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
Optimize will_wake implementation by comparing vtable address instead of its content. The existing best practice to avoid false negatives from will_wake is to define a waker vtable as a static item. That approach continues to works with the new implementation. While this potentially changes the observable behaviour, the function is documented to work on a best-effort basis. The PartialEq impl for RawWaker remains as it was.
|
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks. r? libs |
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (62fb0db): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 637.562s -> 638.252s (0.11%) |
Nadrieril
added a commit
to Nadrieril/rust
that referenced
this pull request
Mar 2, 2024
Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake Fixes rust-lang#121600. As `@jkarneges` identified in rust-lang#121600 (comment), the issue is two different const promotions produce two statics at different addresses, which may or may not later be deduplicated by the linker (in this case not). Prior to rust-lang#119863, the content of the statics was compared, and they were equal. After, the address of the statics are compared and they are not equal. It is documented that `will_wake` _"works on a best-effort basis, and may return false even when the Wakers would awaken the same task"_ so this PR fixes a quality-of-implementation issue, not a correctness issue.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 3, 2024
Rollup merge of rust-lang#121622 - dtolnay:wake, r=cuviper Preserve same vtable pointer when cloning raw waker, to fix Waker::will_wake Fixes rust-lang#121600. As `@jkarneges` identified in rust-lang#121600 (comment), the issue is two different const promotions produce two statics at different addresses, which may or may not later be deduplicated by the linker (in this case not). Prior to rust-lang#119863, the content of the statics was compared, and they were equal. After, the address of the statics are compared and they are not equal. It is documented that `will_wake` _"works on a best-effort basis, and may return false even when the Wakers would awaken the same task"_ so this PR fixes a quality-of-implementation issue, not a correctness issue.
akoshelev
added a commit
to akoshelev/raw-ipa
that referenced
this pull request
May 3, 2024
We used `will_wake` in correctness checks to make sure we don't hang the future/stream forever if a wrong waker is used, but the contract for this function is best-effort and it started generating more and more false positives. The recent one, found by @aleiserson, is inside rust-lang (rust-lang/rust#119863), so it is very hard to get around. We don't have a good replacement for this check unless we implement our own waker, but it is probably too much for now
akoshelev
added a commit
to private-attribution/ipa
that referenced
this pull request
May 3, 2024
We used `will_wake` in correctness checks to make sure we don't hang the future/stream forever if a wrong waker is used, but the contract for this function is best-effort and it started generating more and more false positives. The recent one, found by @aleiserson, is inside rust-lang (rust-lang/rust#119863), so it is very hard to get around. We don't have a good replacement for this check unless we implement our own waker, but it is probably too much for now
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.
Optimize will_wake implementation by comparing vtable address instead of its content.
The existing best practice to avoid false negatives from will_wake is to define a waker vtable as a static item. That approach continues to works with the new implementation.
While this potentially changes the observable behaviour, the function is documented to work on a best-effort basis. The PartialEq impl for RawWaker remains as it was.