Remove lifetime parameters to allow multi-threaded installations

[FranciscoTGouveia]

Cherry-picked from #4471, in response to #4471 (comment).

[rami3l]

LGTM, thanks :)

[djc]

Did you both miss my comment from the other PR opposing these changes? Or did I miss some response to that?

[rami3l]

Did you both miss my comment from the other PR opposing these changes? Or did I miss some response to that?

@djc You have been giving very important and insightful review comments, but I believe we might be having some sort of a misunderstanding here. If you meant #4471 (review):

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.

The latest update of #4471 doesn't use channels anymore (it's not finished as the refactor should be squashed back), but removing lifetimes are still required to send the current transaction to a new task, since without spawning (say up to b5e9e4d), the feature shouldn't work on it's own and no performance boost is introduced.

@FranciscoTGouveia has, in fact, asked me the very question upon finishing the first commit wondering why the performance was not improved; we deduced together and concluded that a separate thread for installation is required because most of it is still sync and, as we have previously agreed, the installer itself will stay sync for now (#4159).

I've always wanted to minimize the diff to land the change, so I understand your concern of certain commits being too huge to revert and/or a move in the wrong direction. Of course we could be missing some crucial factors in your envisioned solution if the changes proposed in this PR are really not required. In that case we'd be happy to know.

[rami3l]

PS: Also, on a general sense, tokio seems to dislike the lifetime dance so I doubt whether changes like this won't spread in the future despite being a bit unfortunate.

[djc]

The latest update of #4471 doesn't use channels anymore (it's not finished as the refactor should be squashed back), but removing lifetimes are still required to send the current transaction to a new task, since without spawning (say up to b5e9e4d), the feature shouldn't work on it's own and no performance boost is introduced.

Ah, cool. I was referring also to #4471 (comment):

If that's the case, I would suggest something more like a FuturesUnordered containing all the downloads, and then pulling from that to run the installations.

So I'm suggesting that downloading happens concurrently by storing a bunch ComponentBinary::download() thingies in a FuturesUnordered, then the main thread/task can pull from those to do installations one at a time.

[rami3l]

So I'm suggesting that downloading happens concurrently by storing a bunch ComponentBinary::download() thingies in a FuturesUnordered

@djc IIRC the latest update of #4471 is doing exactly that.

Are you suggesting something like task::block_in_place() should be used instead of task::spawn_blocking() to avoid sending the installer to another thread?

I'm asking this because we have failed to move downloads to another thread via a JoinSet or similar. It's giving errors like `the trait `std::marker::Sync` is not implemented for `RefCell<Option<&mut CoreWrapper<CtVariableCoreWrapper<..., ..., ...>>>>`, not sure what is happening either.

Update

It looks like the above error comes from here:

rustup/src/download/mod.rs

Lines 92 to 101 in d676c6f

	let hasher = RefCell::new(hasher);
	// This callback will write the download to disk and optionally
	// hash the contents, then forward the notification up the stack
	let callback: &dyn Fn(Event<'_>) -> anyhow::Result<()> = &|msg| {
	if let Event::DownloadDataReceived(data) = msg
	&& let Some(h) = hasher.borrow_mut().as_mut()
	{
	h.update(data);
	}

Either way it seems like it's about doing the remove refcell/lifetime dance either with the downloading or the installing code.

[djc]

Are you suggesting something like task::block_in_place() should be used instead of task::spawn_blocking() to avoid sending the installer to another thread?

I'm suggesting we don't need to send the installer to another thread at all, because the downloads will continue for a bit in the background even if the main task is not polling them.

[rami3l]

I'm suggesting we don't need to send the installer to another thread at all, because the downloads will continue for a bit in the background even if the main task is not polling them.

@djc Given a sync function call blocking a certain thread, how can other tasks make progress if we don't have other threads to use?

If you are talking about sending downloading tasks to other threads first (instead of sending the installer), then we are on the same page now. However, as explained in #4567 (comment), it's not immediately possible either since it requires a dance similar to this PR.

Of course I can experiment on that direction and see which approach will result in a smaller diff.

[djc]

@djc Given a sync function call blocking a certain thread, how can other tasks make progress if we don't have other threads to use?

I'm pretty sure you're modelling async I/O wrong in your head. Once we have initiated a download, TCP frames are going to continue to come in (modulo flow control and congestion management). Thus, we don't need to continuously poll the I/O tasks in order for the downloads to make progress, we can execute on an installation and then poll the downloads again.

If you are talking about sending downloading tasks to other threads first (instead of sending the installer), then we are on the same page now. However, as explained in #4567 (comment), it's not immediately possible either since it requires a dance similar to this PR.

No, as I've tried to explain several times now I'm suggesting we don't send any tasks to other threads.

[rami3l]

@djc Thanks for your clarification. It is true that the downloads that have been initiated will not be influenced, but the expected behavior also includes the normal initiation of new downloads when a component is being unpacked (which is very important when large components such as rust-docs are being installed and small components are being downloaded), as well as download progress reporting in that period.

My confusion is that it seems this cannot be handled using the naive blocking approach, but of course I'd be glad to be corrected.

[djc]

It is true that the downloads that have been initiated will not be influenced, but the expected behavior also includes the normal initiation of new downloads when a component is being unpacked (which is very important when large components such as rust-docs are being installed and small components are being downloaded), as well as download progress reporting in that period.

Okay, that makes a little more sense. I submitted #4570 which substantially simplifies the core goal of concurrent downloading & installation. I suggest we use that as a starting point to figure out what the effect is with large installations like rust-docs and how best to mitigate those effects.