-
Notifications
You must be signed in to change notification settings - Fork 984
Interleave downloads with installations when downloading a toolchain #4471
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
base: main
Are you sure you want to change the base?
Interleave downloads with installations when downloading a toolchain #4471
Conversation
c4b68b2 to
c2954ba
Compare
c2954ba to
1d7e5f5
Compare
0ea9162 to
a2a2c20
Compare
947b8da to
9e5d0e0
Compare
9e5d0e0 to
dd68c17
Compare
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.
Modulo the previously requested changes and the two extra nits, I think this PR is quite ready. Thanks a lot, @FranciscoTGouveia!
d149360 to
1b88c36
Compare
1b88c36 to
5b8e05a
Compare
As I read these results: high speed connections: I suspect some interaction with memory limits showing up in the unpack phase there; I suggest raising the working memory much higher, if you're on a machine that has capacity, to see if thats the case. But also it suggests the default should probably be quite conservative, as we still need to support unpacking on Rasberry PI class machines. |
7228974 to
399b575
Compare
8895d18 to
1830dcb
Compare
|
Hello everyone! I think all review comments should be addressed by now with this and this comments being left for a future PR -- the justification can be seen in the comments.
First of all, thank you for your comment @rbtcollins. I have indeed reran the benchmarks with a higher memory and it didn't show any difference. For reference, it was a machine equipped with an Intel Xeon E5-2630 v2 processor and 64 GB of RAM. Moreover, I would like to thank @djc for all the simplification work that he did, which made this PR much easier to implement. 😄 |
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.
LGTM modulo some questions/nits :)
ea7ab01 to
a18c152
Compare
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.
So while I did say I was fine with doing spawning/channel approach, after looking at the "remove some lifetime parameters" commit I am no longer sure if that's the best approach.
Effectively it means that whoever goes and puts in place the right architecture will basically have to undo all of those changes which IMO should not be necessary.
|
(It might be useful to start split some of the commits out of this PR, since it's obviously complex/contentious -- would be useful to start landing some parts to avoid more rebasing. At least the first commit is an obvious candidate.) |
|
I think the correcting faulty output commits are also independent of the other tricky parts? |
Yes, they are. Would it be okay if I opened a PR for that now to help streamline this one? |
Definitely! |
Even though downloads are done concurrently, the installations are done sequentially. This means that, as downloads complete, they are in a queue (an mpsc channel) waiting to be consumed by the future responsible for the (sequential) installations. There was a need to relax some test cases to allow for uninstall to happen before the downloads.
…ation with a download
…hread installations Co-authored-by: rami3l <rami3l@outlook.com>
…nue during installations Co-authored-by: rami3l <rami3l@outlook.com>
a18c152 to
a25e9c1
Compare


Continuation of #4436.
This change aims to speed up the overall performance of the
rustup [toolchain] installcommand by allowing the installation of a component to start immediately after its download completes.This approach is particularly beneficial for slower connections, as it enables installations to progress in parallel with ongoing downloads -- so by the time all downloads are finished, all installations should be completed as well.
NB: This PR is not yet ready for review. I believe this change should be backed by benchmarks and user feedback before moving forward. I’ll share those here in the next couple of days.