Concurrently download components of a toolchain #4436

FranciscoTGouveia · 2025-08-03T10:10:55Z

As of now, each component of a toolchain is downloaded sequentially, leaving room for performance improvements.
This is particularly notorious when considering interleaving downloads with installation, as discussed in #731.

Following the work made in #4426, as the DownloadTracker is now indicatif-based, doing the progress reporting for concurrent downloads is much easier.
As such, this PR comes to enable downloading different components concurrently (possibly closing #3018).

Performance-wise, benchmarks using hyperfine showed a small (1.1x) speedup -- which doesn't seem noticeable but lays the foundations for a future PR to interleave the download with the installation (of components).
The benchmarks were ran 50 times (with 5 warmup runs) over a 50 Mbps connection, each downloading an entire toolchain.

Many thanks to @rami3l and his PR for refactoring part of the test suite for the purpose of this PR.

FranciscoTGouveia · 2025-08-03T22:44:41Z

For reference, the current download of components (sequential) looks like this:

... and the changes introduced by this PR go like this:

Please note that both animations were sped up and they are not meaningful for comparing performances.

src/cli/download_tracker.rs

src/dist/manifestation.rs

rami3l · 2025-08-10T14:51:22Z

src/utils/notifications.rs

@@ -14,11 +14,11 @@ pub enum Notification<'a> {
    RemovingDirectory(&'a str, &'a Path),
    DownloadingFile(&'a Url, &'a Path),
    /// Received the Content-Length of the to-be downloaded data.
-    DownloadContentLengthReceived(u64),
+    DownloadContentLengthReceived(u64, &'a str),


Would you mind explaining the meaning of "" here and why they are required? It might be an interesting addition to the doc comments as well.

Since we are downloading the components concurrently, we can’t know which download the Content-Length refers to. Therefore, in addition to receiving the length itself, we also need the component’s URL so we can match it to the corresponding progress bar.

Note: I've already added a doc comment. Thank you!

Before resolving this conversation, I will investigate the correct use of this notification and the addition of this extra parameter to ensure that, even though the installations are not concurrent, it did not have any negative effect on them (which, unfortunately, cannot be guaranteed by the test suite).

rami3l · 2025-08-10T15:10:53Z

src/cli/download_tracker.rs

            )
            .unwrap()
            .progress_chars("## "),
        );
+        pb.set_message(component.to_string());


Nit: .to_string() doesn't seem necessary here...

I may be misreading the situation here, but I think we actually need the .to_string() here.

This is due to the fact that the set_message() expects Into<Cow<'static, str>>. and we would try to pass a &str with a non-'static lifetime.

IMO to_owned() more clearly expresses what you're trying to do (and might be slightly more efficient). In this case since create_progress_bar() needs two copies, suggest taking the argument as String and cloning it for use in set_message().

Some notifications needed to be updated to include the download URL, enabling the identification of the component being downloaded. This was necessary for accurate progress reporting of each component.

FranciscoTGouveia · 2025-08-11T19:19:16Z

After my investigation stemming from this comment, I have concluded that the changes presented on this PR are not consistent with the previous behavior of displaying a progress bar during the installation of the component.

Would this be necessary, especially considering that the next step will be to interleave the downloads with the installation?

rami3l · 2025-08-12T11:56:48Z

I have concluded that the changes presented on this PR are not consistent with the previous behavior of displaying a progress bar during the installation of the component.

Would this be necessary, especially considering that the next step will be to interleave the downloads with the installation?

@FranciscoTGouveia No, I don't think it is necessary for that part of the behavior to stay unchanged, as you have already mentioned. Of course, if it is easy to add it back, then it might still be interesting to do so.

rami3l · 2025-08-12T12:04:53Z

src/dist/manifestation.rs


-            things_to_install.push((component, format, downloaded_file));
+        let component_stream =
+            tokio_stream::iter(components.into_iter()).map(|(component, format, url, hash)| {


We also have to consider the restriction of concurrent downloads similar to what we have done with threadpooled diskio.

This closure is complicated enough that I think it should be extracted into a separate function or struct.

djc · 2025-08-12T13:37:21Z

src/cli/download_tracker.rs

+        // During the installation this function can be called with an empty file/URL.
+        if file.is_empty() {
+            return None;
+        }


Why does that happen? Seems pretty iffy.

djc · 2025-08-12T13:39:11Z

src/cli/download_tracker.rs

+        let component = self
+            .file_progress_bars
+            .keys()
+            .find(|comp| file.contains(*comp))?;
+
+        self.file_progress_bars.get(component)


Why this elaborate setup? Are file values not the same as the components?

djc · 2025-08-12T13:39:36Z

src/cli/download_tracker.rs

-        self.multi_progress_bars.remove(&self.progress_bar);
-        self.progress_bar = ProgressBar::hidden();
+    pub(crate) fn download_finished(&mut self, file: &str) {
+        if let Some(pb) = self.progress_bar(file) {


Nit: let Some(pb) = .. else { return };.

djc · 2025-08-12T13:40:16Z

src/cli/download_tracker.rs

+            let msg = pb.message();
+            pb.finish_with_message(msg);


This seems weird? Could instead probably use one of the other ProgressFinish modes.

djc · 2025-08-12T13:41:42Z

src/cli/download_tracker.rs

            )
            .unwrap()
            .progress_chars("## "),
        );
+        pb.set_message(component.to_string());


IMO to_owned() more clearly expresses what you're trying to do (and might be slightly more efficient). In this case since create_progress_bar() needs two copies, suggest taking the argument as String and cloning it for use in set_message().

djc · 2025-08-12T13:42:11Z

src/diskio/threaded.rs

@@ -263,10 +263,11 @@ impl Executor for Threaded<'_> {
        // pretend to have bytes to deliver.
        let mut prev_files = self.n_files.load(Ordering::Relaxed);
        if let Some(handler) = self.notify_handler {
-            handler(Notification::DownloadFinished);
+            handler(Notification::DownloadFinished(""));


This seems strange. Why pass in an empty string?

djc · 2025-08-12T13:42:33Z

src/diskio/threaded.rs

            handler(Notification::DownloadPushUnit(Unit::IO));
            handler(Notification::DownloadContentLengthReceived(
                prev_files as u64,
+                "",


Same here. What does the empty string mean? Should the type be Option<_> instead?

djc · 2025-08-12T13:43:34Z

src/dist/manifestation.rs

+                    Ok::<_, Error>((component, format, downloaded_file, hash))
+                }
+            });
+        if components_len > 0 {


If this is the only use of components_len, suggest storing a bool instead.

djc · 2025-08-12T13:44:38Z

src/dist/manifestation.rs


-            things_to_install.push((component, format, downloaded_file));
+        let component_stream =
+            tokio_stream::iter(components.into_iter()).map(|(component, format, url, hash)| {


This closure is complicated enough that I think it should be extracted into a separate function or struct.

FranciscoTGouveia force-pushed the concurrent-downloads branch 3 times, most recently from 10d1514 to 7093333 Compare August 5, 2025 13:13

FranciscoTGouveia marked this pull request as ready for review August 5, 2025 13:24

rami3l self-assigned this Aug 10, 2025

rami3l reviewed Aug 10, 2025

View reviewed changes

This comment was marked as duplicate.

Sign in to view

rami3l reviewed Aug 10, 2025

View reviewed changes

FranciscoTGouveia force-pushed the concurrent-downloads branch from 7093333 to 837bf58 Compare August 11, 2025 13:53

feat(downloads): concurrently download components

4737f25

Some notifications needed to be updated to include the download URL, enabling the identification of the component being downloaded. This was necessary for accurate progress reporting of each component.

FranciscoTGouveia force-pushed the concurrent-downloads branch from 837bf58 to 4737f25 Compare August 11, 2025 14:21

rami3l reviewed Aug 12, 2025

View reviewed changes

djc reviewed Aug 12, 2025

View reviewed changes

Concurrently download components of a toolchain #4436

Are you sure you want to change the base?

Concurrently download components of a toolchain #4436

Conversation

FranciscoTGouveia commented Aug 3, 2025 • edited by rami3l Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FranciscoTGouveia commented Aug 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranciscoTGouveia commented Aug 11, 2025

Uh oh!

rami3l commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FranciscoTGouveia commented Aug 3, 2025 •

edited by rami3l

Loading

rami3l commented Aug 12, 2025 •

edited

Loading