Run a parallel pre-check before updating toolchains#4752
Run a parallel pre-check before updating toolchains#4752FranciscoTGouveia wants to merge 1 commit intorust-lang:mainfrom
Conversation
src/cli/common.rs
Outdated
| future::join_all(channels.iter().map(|(_, d)| d.show_dist_version())) | ||
| .await | ||
| .into_iter() | ||
| .map(|v| v.map_or(true, |opt| opt.is_some())) | ||
| .collect() | ||
| }; |
There was a problem hiding this comment.
Is there a cleaner way to achieve this?
There was a problem hiding this comment.
Am I reading it right that now for every toolchain that needs to be updated, the update is checked twice?
There was a problem hiding this comment.
Yes, I forgot to add a comment about this, sorry.
I see two possible ways to proceed:
- Accept the cost of performing a double check. In practice, since the first check is done concurrently in a batch, the overhead should be relatively small.
- Do a small refactor, starting in
DistOptions::install_into(), to allow skipping the second check.
There was a problem hiding this comment.
@FranciscoTGouveia I think we should try to avoid double checking if the changes involved are not super disruptive 🙏
| // Run a pre-check to determine which channels have updates available. | ||
| // This step is skipped when force-updating. |
There was a problem hiding this comment.
Feedback on the wording of the comment is welcome :)
FranciscoTGouveia
force-pushed
the
update-performance
branch
from
18df18c to
d6cc274
Compare
| join_all(channels.iter().map(|(_, d)| d.show_dist_version())) | ||
| .await | ||
| .into_iter() | ||
| .map(|v| v.map_or(true, |opt| opt.is_some())) |
There was a problem hiding this comment.
I'm confused by this logic. show_dist_version() seems to merely yield the latest available version, but that a DistributableToolchain has a manifest with a rust component with a version is_some() doesn't mean that it's newer than the installed version?
There was a problem hiding this comment.
Sorry, I do not think I fully understood your comment.
The rationale for using show_dist_version() comes from how it's used in rustup check.
As I understand it, show_dist_version() only returns Some when the hashes do not match (i.e., there is an update available), which aligns with what we need here.
In this setup, for toolchains that do not require updating, we would never reach .get_rust_version(), since .dl_v2_manifest() returned None.
Apologies if I've misunderstood or overlooked anything.
There was a problem hiding this comment.
As I understand it,
show_dist_version()only returnsSomewhen the hashes do not match (i.e., there is an update available), which aligns with what we need here.
You're completely right. Might be good to add a docstring for show_dist_version() and maybe consider renaming it.
There was a problem hiding this comment.
Sounds good, I will take care of it. :)
3 participants
Fixes #4747.
Instead of sequentially checking each toolchain for updates,
rustup updatenow first checks all toolchains concurrently and only then performs the updates (if needed).This brings two advantages:
rustup updateis now ~2x faster (see below);nightlyneeds updating.Benchmarks were run using
hyperfine(20 iterations on a 50 Mbps connection) on a setup with 11 toolchains, and no regressions were observed.For the no-op
rustup update, this patch was 2.2x faster; for cases where a single toolchain was updated, there was no measurable difference.CC @epage