-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix(registry): HttpRegistry
block_until_ready
returns early when work is still pending
#14694
Conversation
does the code cargo/src/cargo/core/package.rs Line 960 in 8040c00
|
Seem pretty likely, though this is downloading the package so the perf gain may not be as significant as registry index one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏾
Good either with or without #14694 (comment).
The current implementation looks correct to me. It doesn't have the same bug with early returning when there is still work to be done. My notes on how it's working:
|
@bors r=weihanglo |
☀️ Test successful - checks-actions |
Update cargo 7 commits in 8c30ce53688e25f7e9d860b33cc914fb2957ca9a..cf53cc54bb593b5ec3dc2be4b1702f50c36d24d5 2024-10-15 16:43:16 +0000 to 2024-10-18 13:56:15 +0000 - feat: Stabilize MSRV-aware resolver config (rust-lang/cargo#14639) - Help with `[patch.crates.io]` (rust-lang/cargo#14700) - test: Migrate publish snapshotting to snapbox (rust-lang/cargo#14642) - Bump to 0.85.0; update changelog (rust-lang/cargo#14695) - Fix typo in faq.md (rust-lang/cargo#14696) - fix(registry): `HttpRegistry` `block_until_ready` returns early when work is still pending (rust-lang/cargo#14694) - fix(resolver): avoid cloning when iterating using RcVecIter (rust-lang/cargo#14690)
What does this PR try to resolve?
block_until_ready
is returning early when there are still pending transfers. This leads to extra restarts of the resolver, impacting performance.In the existing implementation:
handle_completed_downloads
runs and there are no completed downloadsmulti.perform()
is called and completes new downloads, with no pending transfers remainingremaining_in_multi
, the function returns earlyThis fixes the issue by reordering the calls to
multi.perform()
,handle_completed_downloads
, then the completion check.How should we test and review this PR?
A test is added that uses cargo's tracing to show the number of blocking calls. First commit shows existing behavior, second commit shows fix.
Originally based on the PR #14680 by @x-hgg-x. The ordering fix in this PR also avoids an additional
block_until_ready
call for retried failed downloads.