Skip to content

bootstrap/format: send larger batches to rustfmt#121781

Merged
bors merged 1 commit intorust-lang:masterfrom
RalfJung:bootstrap-fmt
Feb 29, 2024
Merged

bootstrap/format: send larger batches to rustfmt#121781
bors merged 1 commit intorust-lang:masterfrom
RalfJung:bootstrap-fmt

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 29, 2024

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 •
edited
Loading

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

clubby789 commented Feb 29, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

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
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 29, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
@bors
Auto merge of rust-lang#121800 - GuillaumeGomez:rollup-wob2qcz, r=Gui…
384d26f 
…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 •
edited
Loading

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
@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
@rust-timer
Unrolled build for rust-lang#121781
aa2d68b 
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 •
edited
Loading

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 •
edited
Loading

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

RalfJung commented Feb 29, 2024

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

Reviewers

No reviews

Assignees

@clubby789 clubby789

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

Milestone

1.78.0

Development

Successfully merging this pull request may close these issues.

6 participants

@RalfJung @rustbot @Urgau @clubby789 @bors @the8472