Waker::will_wake: Compare vtable address instead of its content by tmiasko · Pull Request #119863 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation7 Commits1 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
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.
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)
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
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
📌 Commit 9d84589 has been approved by m-ou-se
It is now in the queue for this repository.
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
Finished benchmarking commit (62fb0db): comparison URL.
Overall result: ❌ regressions - no action needed
@rustbot label: -perf-regression
Instruction count
This is a highly reliable metric that was used to determine the overall result at the top of this comment.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 6.5% | [6.5%, 6.5%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
Results
This 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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | - | - | 0 |
| Improvements ✅ (primary) | -2.6% | [-2.6%, -2.6%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | -2.6% | [-2.6%, -2.6%] | 1 |
Cycles
Results
This 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.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 4.9% | [4.9%, 4.9%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 637.562s -> 638.252s (0.11%)
Artifact size: 306.31 MiB -> 306.28 MiB (-0.01%)
Nadrieril added a commit to Nadrieril/rust that referenced this pull request
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
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
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
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…cuviper
make LocalWaker::will_wake consistent with Waker::will_wake
This mirrors rust-lang#119863 for LocalWaker. Looks like that got missed in the initial LocalWaker PR (rust-lang#118960).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…cuviper
make LocalWaker::will_wake consistent with Waker::will_wake
This mirrors rust-lang#119863 for LocalWaker. Looks like that got missed in the initial LocalWaker PR (rust-lang#118960).
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#128882 - RalfJung:local-waker-will-wake, r=cuviper
make LocalWaker::will_wake consistent with Waker::will_wake
This mirrors rust-lang#119863 for LocalWaker. Looks like that got missed in the initial LocalWaker PR (rust-lang#118960).
RalfJung pushed a commit to RalfJung/miri that referenced this pull request
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the library team, which will review and decide on the PR/issue.