Skip to content

Conversation

Shourya742
Copy link
Contributor

This PR removes start_process utility method.

r? @Kobzol

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 14, 2025
@Kobzol
Copy link
Member

Kobzol commented Jun 14, 2025

Thanks! Could you please do a small benchmark to check how long it takes to run all the gitinfos now? Before, these three commands executed in parallel, while now they run sequentially. I don't expect a lot of changes, but we do invoke a bunch of gitinfos, so it would be nice to check.

@bors
Copy link
Collaborator

bors commented Jun 14, 2025

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

@the8472
Copy link
Member

the8472 commented Jun 14, 2025

I itentionally added this as a perf optimization in #131954, is there a problem with this method existing?

@Kobzol
Copy link
Member

Kobzol commented Jun 14, 2025

We're trying to get rid of various adhoc ways of executing commands in bootstrap, and instead use a unified API so that we can get caching/tracing/mocking.

We already noticed that this helps perf. a bit, yeah. We'll add some API to allow batching commands to keep the current behavior. In fact, I think we could parallelize across all GitInfo invocations, we do a lot of them.

@the8472
Copy link
Member

the8472 commented Jun 14, 2025

Ok, the PR didn't provide any context, so it looked like a change without much benefit.

@Kobzol
Copy link
Member

Kobzol commented Jun 16, 2025

Obsoleted by #142591.

@Kobzol Kobzol closed this Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants