Skip to content
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

WIP: optimize process spawning on Linux #118750

Closed
wants to merge 1 commit into from

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Dec 8, 2023

This PR optimizes process spawning on Linux by avoiding allocations and sorting when copying environment variables. The code is quite WIP, as I'm not sure where should I put this optimized code (process_unix, process_common?).

I wasn't able to get large speedups by just removing the BTreeMap, so I went all the way and tried to remove as many allocations as possible. I created a simple benchmark here that spawns 8k processes, each with ~200 environment variables. Locally, on my Linux 5.15.0 and glibc 2.35, the benchmark goes from ~1.4s to ~1s after the changes. This is of course a massive stress test for command spawning, for most situations, the perf. improvements will be negligible.

One behaviour change of this PR is that the environment variables are no longer sorted before being passed to the spawned command, but I don't think that it's very important on Linux.

The motivation for this PR is that process spawning on Linux can be quite slow if there are a lot of environment variables, you don't use Command::env_clear() and you set some environment variables. In that case, all existing environment variables will be copied (several times), which can be a bottleneck if there are a lot of variables. This has occured to me in HyperQueue, which spawns a lot of processes, and this turned out to be a bottleneck in some cases. Some discussion of this problem has happened on Zulip.

@rustbot
Copy link
Collaborator

rustbot commented Dec 8, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 8, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

By avoiding allocations and sorting when copying environment variables
Add Rust CI workflow analysis
@bors
Copy link
Contributor

bors commented Jan 13, 2024

☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts.

@cuviper
Copy link
Member

cuviper commented Jan 25, 2024

I'm clearing this from my queue while it's in draft, but you can use ready when you are.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2024
@Dylan-DPC
Copy link
Member

@Kobzol any updates on this? thanks

@Kobzol
Copy link
Contributor Author

Kobzol commented Feb 19, 2024

I'm going to close this for now, it was only a win in microbenchmarks, and I'm not sure if it's worth the complication.

@Kobzol Kobzol closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants