Fix mpsc::SyncSender spinning behavior by ibraheemdev · Pull Request #106701 · 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 Commits3 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 }})
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
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK std::sync::mpmc does not use is_completed together with snooze, so this looks like the correct fix for the current implementation of std::sync::mpmc.
Resolves #106668.
I think beta backport is also needed to completely fix #106668 because it marked as regression-from-stable-to-beta.
The current documentation around Backoff seems to be inconsistent with how it is actually used in the mpmc implementation, so (if we keep the current behavior) I would suggest updating it.
| /// Backs off in a lock-free loop. |
|---|
| /// |
| /// This method should be used when we need to retry an operation because another thread made |
| /// progress. |
| /// Backs off in a blocking loop. |
|---|
I reviewed the current usage and think it is fine.
@bors r+
Separately, I think the retry loop when sending a message should be removed (like we did with the similar loop in recv). The reasoning is the same: we don't want to waste CPU cycles spinning when trying to write to a full channel. Instead we should just go ahead and block immediately. But this can be a separate PR.
📌 Commit 8917e99 has been approved by Amanieu
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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 10 pull requests
Successful merges:
- rust-lang#104645 (Add log-backtrace option to show backtraces along with logging)
- rust-lang#106465 (Bump
IMPLIED_BOUNDS_ENTAILMENTto Deny + ReportNow) - rust-lang#106489 (Fix linker detection for linker (drivers) with a version postfix (e.g. clang-12 instead of clang))
- rust-lang#106585 (When suggesting writing a fully qualified path probe for appropriate types)
- rust-lang#106641 (Provide help on closures capturing self causing borrow checker errors)
- rust-lang#106678 (Warn when using panic-strategy abort for proc-macro crates)
- rust-lang#106701 (Fix
mpsc::SyncSenderspinning behavior) - rust-lang#106793 (Normalize test output more thoroughly)
- rust-lang#106797 (riscv: Fix ELF header flags)
- rust-lang#106813 (Remove redundant session field)
Failed merges:
r? @ghost
@rustbot modify labels: rollup
Separately, I think the retry loop when sending a message should be removed (like we did with the similar loop in recv). The reasoning is the same: we don't want to waste CPU cycles spinning when trying to write to a full channel. Instead we should just go ahead and block immediately. But this can be a separate PR.
I agree, the original reasoning was that it is similar to acquiring a mutex, but the difference is that bounded channels are often used with heavy backpressure, so a quick unpark is often not expected.
bors added a commit to rust-lang-ci/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request
Labels
Accepted for backporting to the compiler in the beta channel.
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.