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

bootstrap/format: send larger batches to rustfmt #121781

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

RalfJung
Copy link
Member

This helps on systems with low core counts. To benchmark this I made a lot of files be modified:

for FILE in $(find compiler/ -name "*.rs"); do echo "// end of the file" >>$FILE; done

Then I ran

hyperfine "./x.py fmt -j1" -w 1 -r 4

Before this patch:

Benchmark 1: ./x.py fmt -j1
  Time (mean ± σ):      3.426 s ±  0.032 s    [User: 4.681 s, System: 1.376 s]
  Range (min … max):    3.389 s …  3.462 s    4 runs

With this patch:

Benchmark 1: ./x.py fmt -j1
  Time (mean ± σ):      2.530 s ±  0.054 s    [User: 4.042 s, System: 0.467 s]
  Range (min … max):    2.452 s …  2.576 s    4 runs

@rustbot
Copy link
Collaborator

rustbot commented Feb 29, 2024

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 29, 2024
@Urgau
Copy link
Member

Urgau commented Feb 29, 2024

Did you try with std::thread::available_parallelism ? Did it make a difference ?

@RalfJung
Copy link
Member Author

RalfJung commented Feb 29, 2024

This code quite deliberately honors -j, I think. -j already defaults to available_parallelism.

On my system, available_parallelism is 20, so that just means we're no longer measuring what happens on low-core systems.

@clubby789
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 29, 2024

📌 Commit c54d92c has been approved by clubby789

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
…llaumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#118217 (Document which methods on `f64` are precise)
 - rust-lang#119748 (Increase visibility of `join_path` and `split_paths`)
 - rust-lang#121412 (platform docs: clarify hexagon-unknown-none-elf example, add hexagon-unknown-linux-musl)
 - rust-lang#121654 (Fix `async Fn` confirmation for `FnDef`/`FnPtr`/`Closure` types)
 - rust-lang#121700 (CFI: Don't compress user-defined builtin types)
 - rust-lang#121765 (add platform-specific function to get the error number for HermitOS)
 - rust-lang#121781 (bootstrap/format: send larger batches to rustfmt)
 - rust-lang#121788 (bootstrap: fix clap deprecated warnings)
 - rust-lang#121792 (Improve renaming suggestion when item starts with underscore)
 - rust-lang#121793 (Document which methods on `f32` are precise)

r? `@ghost`
`@rustbot` modify labels: rollup
@the8472
Copy link
Member

the8472 commented Feb 29, 2024

On my system, available_parallelism is 20, so that just means we're no longer measuring what happens on low-core systems.

The batch size was set to a small value because it might end up distributing work unevenly across rustfmt processes and thus lead to worse wall-time on many-core systems.
Have you measured the impact without -j 1?

Additionally, very little hardware with only a single core exists these days, so I think something like -j 4 would be more realistic.

Have you tried smaller increments than jumping from 8 to 64?

@bors bors merged commit b52b699 into rust-lang:master Feb 29, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 29, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
Rollup merge of rust-lang#121781 - RalfJung:bootstrap-fmt, r=clubby789

bootstrap/format: send larger batches to rustfmt

This helps on systems with low core counts. To benchmark this I made a lot of files be modified:
```
for FILE in $(find compiler/ -name "*.rs"); do echo "// end of the file" >>$FILE; done
```
Then I ran
```
hyperfine "./x.py fmt -j1" -w 1 -r 4
```
Before this patch:
```
Benchmark 1: ./x.py fmt -j1
  Time (mean ± σ):      3.426 s ±  0.032 s    [User: 4.681 s, System: 1.376 s]
  Range (min … max):    3.389 s …  3.462 s    4 runs
```
With this patch:
```
Benchmark 1: ./x.py fmt -j1
  Time (mean ± σ):      2.530 s ±  0.054 s    [User: 4.042 s, System: 0.467 s]
  Range (min … max):    2.452 s …  2.576 s    4 runs
```
@RalfJung
Copy link
Member Author

RalfJung commented Feb 29, 2024

Without -j1 it's the same up to noise:
Before:

Benchmark 1: ./x.py fmt
  Time (mean ± σ):     841.1 ms ±  25.1 ms    [User: 7214.1 ms, System: 1635.5 ms]
  Range (min … max):   804.1 ms … 860.0 ms    4 runs

After:

Benchmark 1: ./x.py fmt
  Time (mean ± σ):     851.2 ms ±  13.4 ms    [User: 6079.3 ms, System: 545.0 ms]
  Range (min … max):   832.0 ms … 861.1 ms    4 runs

I think in most cases it'll be much less than 63 files since the formatting workers will be draining the queue faster than the directory walker fills it. I haven't tried to measure that though.

Have you tried smaller increments than jumping from 8 to 64?

I incremented it until there was no significant improvement with -j1 any more. (For the next step, to 128, I also increased the queue size since if the queue is capped at 128 even with two workers it seems unlikely they will both get 128 jobs.)

@the8472
Copy link
Member

the8472 commented Feb 29, 2024

Ok, sounds good.

@the8472
Copy link
Member

the8472 commented Feb 29, 2024

Actually, how did you benchmark this? I'm currently looking at the bootstrap rustfmt code and its behavior depends on the git state.
If you're on a clean master then it checks everything.
If you're on a branch or you have modified any files it only checks modified files.

This can taint benchmark results.

Edit, nevermind, I see you modified the files.

do echo "// end of the file" >>$FILE; done

@RalfJung
Copy link
Member Author

Please read the PR description, I described my benchmarking methodology. :)

@RalfJung RalfJung deleted the bootstrap-fmt branch March 2, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants