Open pitaj opened 8 months ago
I'd like to see some benchmarks for the impact this would have on proxy overhead. Like, even in the most optimistic scenario of spawning a different process that handles most of the work asynchronously, that might be prohibitive (at least from being opt-out)? I also feel like this should not be a separate question from the initial prompt, it should be listed as an installation option so that the defaults path doesn't get longer/more elaborate.
I'd like to see some benchmarks for the impact this would have on proxy overhead. Like, even in the most optimistic scenario of spawning a different process that handles most of the work asynchronously, that might be prohibitive (at least from being opt-out)?
Are you looking for proxy-to-cargo latency, or are you talking about performance overhead?
I also feel like this should not be a separate question from the initial prompt, it should be listed as an installation option so that the defaults path doesn't get longer/more elaborate.
My thought process was this:
Essentially, make it as close to opt-in as possible, while still defaulting to what we recommend.
That said, it might not be a problem at all, but I've seen backlash in the past.
Are you looking for proxy-to-cargo latency, or are you talking about performance overhead?
If someone runs cargo --version
, how much slower does that get?
My thought process was this:
I don't think the amount of controversy will change substantially based on whether this is an extra question (with an opt-in default) or part of the initial installation options (with the same opt-in default). We shouldn't make the UX worse.
If someone runs
cargo --version
, how much slower does that get?
Okay, I'll start by measuring the overhead that's already added by the proxy.
That will give a baseline on which to judge what impact 100ms would have.
Then I can make some quick changes to see what overhead just spawning a process would add.
I've measured the proxy latency by replacing the cargo proxy with a stub Rust program that just prints out the system time. I compare the system time from immediately before executing the cargo proxy to the time printed out by the stub.
Results from 1000 samples:
Average [ms] | Min [ms] | Max [ms] | |
---|---|---|---|
Proxy -> stub | 28.345899 | 26.674342 | 31.096926 |
Stub only | 0.542705 | 0.508401 | 1.119083 |
So if we implement a 100ms timeout it would roughly 5x the latency. To be clear, that price would only be paid once a day at the most. Rustup overhead is already multiple orders of magnitude greater than a process spawn.
I will now fork Rustup and see if I can implement something quick and dirty to test this more realistically.
Note: Some say we're not doing well enough in terms of speed, see #2626.
A different approach which fits into some other vague thoughts we've had would be to have a rustup daemon that is running persistently; such a daemon could start implicitly when run, and take care of things like this update, but also permit other features like excluding concurrent use during updates, and avoid manifest parsing in each proxy invocation, which would drop the proxy latency substantially.
Adding a process spawn for an update check in the background appears to be insignificant, I wasn't able to measure a difference between that and the normal proxy latency.
But adding a blocking download-and-parse of the manifest (without even a process spawn) extended the proxy latency to almost 400ms. So the 200ms timeout won't work, unless we add a special endpoint just for update checking (and even that might not be enough).
For me, even a second of wait time would be acceptable if it only happened once a week. But I wouldn't be opposed to always running the check/download in the background - whether that be by a daemon or not.
(Side note: where does the Rustup team communicate? I don't see a zulip channel and it looks like the channel on the Discord either doesn't exist or is private)
(Side note: where does the Rustup team communicate? I don't see a zulip channel and it looks like the channel on the Discord either doesn't exist or is private)
@pitaj We have wg-rustup on Discord and we do welcome everyone to ask their questions... Maybe it's just an oversight?
But adding a blocking download-and-parse of the manifest (without even a process spawn) extended the proxy latency to almost 400ms. So the 200ms timeout won't work, unless we add a special endpoint just for update checking (and even that might not be enough).
The check could run concurrently, taking its time and writing to disk so that the next cargo invocation can see that and display it to the user. I don't think this is something that should run every day. At most once a week, at least once every 3/6 months. Once a month sounds reasonable: faster than the release schedule to catch dot releases. I think I would be ok with a purely time based check every, lets say ~12 weeks (2 releases).
The check could run concurrently, taking its time and writing to disk so that the next cargo invocation can see that and display it to the user.
Yes, there are plenty of alternatives ranging from daemons to what you describe. Many were already suggested in the IRLO thread. I probably should have included more than one option in the original issue.
What you describe is actually the same as what I describe, under the condition that the blocking timeout = 0.
I don't think this is something that should run every day. At most once a week, at least once every 3/6 months. Once a month sounds reasonable: faster than the release schedule to catch dot releases. I think I would be ok with a purely time based check every, lets say ~12 weeks (2 releases).
Weekly is the best default, IMO. I think monthly is too slow, but ultimately it should be the user's choice. Down the line, it would be nice to support daily checks for the nightly toolchain anyways. Even on a daily basis, the check is practically zero cost.
I think we should not do this. I think it will backfire and cause people to become more reluctant to update the toolchain, and/or make them not want to use Rust at all.
There are any number of perfectly good reasons why someone might want to stick to an old version of the toolchain, even a version that has fallen out of support. I estimate that a solid majority of all the Rust users I know personally would hate this notification and would immediately look for a way to turn it off. Some of them would file bugs asking for it to be ripped back out again. A larger group of people would mutter dark thoughts about the software industry's general habit of not respecting user consent and forcing unwanted "feature" upgrades on end users, and some of them would count this as a reason to consider Rust untrustworthy for their purposes.
Furthermore, users have strong expectations about the predictability of whether a command line tool starts a background process (especially a long-lived one) or talks to the network. For an existing tool like rustup
or cargo
, it's OK to add a new subcommand that does one of those things, but it is not OK to make an existing subcommand start doing one of those things as a side effect. Even if I didn't know a lot of people who want to stick to old versions of Rust, I would be opposed to this proposal for that reason alone.
[EDIT: To be crystal clear, I recognize that the proposal is not to automatically upgrade, only to notify of available upgrades. That is already a step too far in my opinion, for the reasons above.]
Your argument seems to only apply if the option is opt-out. If it was opt-in, would that be acceptable?
What are your thoughts on having an extra question on install?
Edit:
There are any number of perfectly good reasons why someone might want to stick to an old version of the toolchain, even a version that has fallen out of support.
I will just note that the solution to this is a rust-toolchain
file or setting an explicit version as the default in rustup. Not upgrading the stable/beta/nightly channel is a very fragile way of accomplishing that goal.
If it was opt-in, would that be acceptable?
Sure, but anyone who wants this can set up a cron job to run rustup update --check-only
at whatever interval they feel is desirable, so I don't really see the point of building it into the tool.
Not upgrading the stable/beta/nightly channel is a very fragile way of [sticking to an old version of the toolchain]
Good point! Now I am tempted to send a PR to change the default behavior of rustup
so that when you install stable the first time it pins that version of stable, and you have to explicitly ask for feature updates.
Sure, but anyone who wants this can set up a cron job to run
rustup update --check-only
at whatever interval they feel is desirable, so I don't really see the point of building it into the tool.
For "prior art", see:
$ pip install pycowsay
Defaulting to user installation because normal site-packages is not writeable
Collecting pycowsay
Using cached pycowsay-0.0.0.2-py3-none-any.whl (4.0 kB)
Installing collected packages: pycowsay
Successfully installed pycowsay-0.0.0.2
[notice] A new release of pip is available: 23.1 -> 24.0
[notice] To update, run: python3 -m pip install --upgrade pip
Ok, pip is far from being the tool with the best user experience in general, but I don't recall hearing any particular pushback on that particular feature. And personally I don't find a nudge to upgrade intrusive at all, as long as it remains short and doesn't affect responsiveness noticeably. (It seems like a nice way to keep the ecosystem up-to-date; in other languages cough C++, people keep using compilers from decades ago because, you know, people tend to just not upgrade unless they're forced to, and this slows down the rate at which language features can be adopted by libraries dramatically.)
Problem you are trying to solve
It is not uncommon for Rust users to have old version of Rust installed without realizing it. This is problematic for users:
It's especially important for users to be notified of "patch" releases which often roll out only when there is a severe issue with a "minor" release toolchain.
Solution you'd like
Essentially, regularly check for toolchain updates and notify users when they run a
rustup
command or when they run acargo
command through the rustup proxy.Prompt on Rust install
On install, users should be asked if they want Rustup to periodically check for updates when invoking
cargo
andrustup
commands:Notice when running command
When a
rustup
command is executed or anycargo
command is executed via the Rustup proxy on thestable
toolchain, begin checking for updates in a background process. Wait with a short timeout; if the check has not completed by then, display a message to the user on the next invocation. If the update check did complete in time, directly display a message to the user.The message to display:
Update packages will be downloaded in the background.
would obviously not be shown if the user hadn't enabled downloading.rustup update --notice
The
-N
flag (short for--notice
) tells Rustup to only run the update that the user was informed of in the most recent notice. If the download was not started, thenrustup update -N
will run essentially the same as arustup update stable
does today: starting the downloads, wait for them to complete, then install them. If the download started but not completed, thenrustup update -N
will complete the download and install the update. If the download has already completed, then it will just install the downloaded update.If a background-downloaded update was never installed and a newer version is detected, the older download will be deleted.
Notes
Timeout duration
We want something long enough to allow the update check to complete for most users, but imperceptible if the update check takes a long time. Something around 100ms-200ms.
Display notice after
cargo
commandWe may want the Rustup notice to show after the cargo command runs so the notice doesn't get buried in the cargo output. However, we can't directly print the notice from rustup without changing the way the cargo proxies work. Instead,
cargo
could add a special command-line option or environment variable for an info item if prints out at the end. It could work something like this:Of course, this would not be compatible with
cargo run
, so we'd need to do some amount of argument parsing. It could be very basic though, even just checking forrun
.How often to display the notice
We don't want to spam the user with notices, otherwise they'll just learn to ignore them. A reasonable behavior might be: