[sled-diagnostics] use JoinSet for multiple commands

[papertigers]

As a part of the #8166 investigation we decided that we should move from FuturesUnordered to a JoinSet to gain parallelism.

[hawkw]

one thought, I'm not sure whether it makes a meaningful difference given that it seems much of the issue is just "fork sucks when you have a big resident set"...

[hawkw]

So, this will not even spawn the tasks until there's capacity in the concurrency limit. What happens if you change it to:

Suggested change

	let permit = Arc::clone(&self.semaphore)
	.acquire_owned()
	.await
	.expect("semaphore acquire");
	let _abort_handle = self.set.spawn(async move {
	let semaphore = self.semaphore.clone();
	let _abort_handle = self.set.spawn(async move {
	let permit = semaphore.acquire().await.expect("semaphore acquire");

This way, all the tasks are spawned immediately, and the task adding commands to the set can do so synchronously (changing add_command to a normal fn) and then just wait for them to all come back. Right now, the task that adds commands has to get woken up a bunch of times to add the next one to the set; I wonder whether changing this to spawn everything synchronously would make a meaningful difference in performance...

[papertigers]

With this change I see the following...

stable rust + 1gb RSS:

took 24.416534527s

beta rust + 1gb RSS:

took 3.829577938s

Over chat I told you that with beta rust and the joinset we saw took 3.674288649s so it's negligible but overall I think a better design to go with your suggestion. I am going to push a commit to this branch.

[hawkw]

ah well, it was worth a shot, i guess! thanks for giving it a spin!

[papertigers]

Test failure filed as #8170
Rerunning the failed test....

[hawkw]

Modulo fixing CI, this looks good to me. I left some non-blocking thoughts.

[hawkw]

How did we arrive at this number?

[hawkw]

On the subject of the concurrency limit, it seems to me that the functions in this crate that run multiple commands fall into two categories: those that run a fairly small, fixed number of commands, like ipadm_info and dladm_info, and those which run a command or set of commands against every Oxide process PID (pargs_oxide_processes etc).

For the functions that run commands against every Oxide process pid, the concurrency limit is certainly useful, as there may be basically any number of pids. But something like ipadm_info will always spawn exactly 3 processes, which is below the concurrency limit, and all this faffing around with a Semaphore is unnecessary.

I kind of wonder if the class of functions that spawn a fixed set of commands should eschew the use of MultipleCommands and just construct a JoinSet and spawn their 3 or 5 tasks or whatever. In practice, any overhead from the semaphore acquire/release/drop and stuff is probably insignificant compared to "actually spawning a child process" so this probably doesn't actually matter, but we could avoid doing it...up to you.

[hawkw]

Looks like the rebase picked up new code that uses FuturesUnordered: https://github.com/oxidecomputer/omicron/actions/runs/15056224877/job/42322633598?pr=8151#step:11:767

[smklein]

FWIW, I pulled this code into #8174, and added a test that we don't exceed our specified "parallelism limit" there.

[papertigers]

FWIW, I pulled this code into #8174, and added a test that we don't exceed our specified "parallelism limit" there.

I can wait to merge this then, thanks for pulling this into a stand alone crate. Let's just use your new crate when it lands instead of duplicating the code.