Skip to content
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

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Oct 15, 2024

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 downloads
  • multi.perform() is called and completes new downloads, with no pending transfers remaining
  • Since there are no items remaining_in_multi, the function returns early

This 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.

@rustbot
Copy link
Collaborator

rustbot commented Oct 15, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024
@Eh2406
Copy link
Contributor

Eh2406 commented Oct 15, 2024

does the code

self.add_sleepers()?;
also need adjustment?

@weihanglo
Copy link
Member

does the code

self.add_sleepers()?;

also need adjustment?

Seem pretty likely, though this is downloading the package so the perf gain may not be as significant as registry index one.

Copy link
Member

@weihanglo weihanglo left a 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).

@arlosi
Copy link
Contributor Author

arlosi commented Oct 15, 2024

does the code

self.add_sleepers()?;

also need adjustment?

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:

loop {
- wait()
  - wait_for_curl() 
     loop {
     - add_sleepers() [self.sleeping -> self.pending (may not move all, depending on timing)]
     - multi.perform() [self.pending -> self.results]
     }
  - self.pending.remove()
  - self.sleeping.push() [for failed downloads that need retry]
}

@arlosi
Copy link
Contributor Author

arlosi commented Oct 15, 2024

@bors r=weihanglo

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

📌 Commit 8db2192 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Oct 15, 2024
@bors
Copy link
Collaborator

bors commented Oct 15, 2024

⌛ Testing commit 8db2192 with merge 79a491a...

@bors
Copy link
Collaborator

bors commented Oct 15, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 79a491a to master...

@bors bors merged commit 79a491a into rust-lang:master Oct 15, 2024
22 checks passed
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 19, 2024
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)
@rustbot rustbot added this to the 1.84.0 milestone Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants