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

Use more threads when discovering python files #12258

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jul 9, 2024

Summary

The ignore crates defaults to two threads when using build_parallel. This PR adopts a simplified heuristic from ripgrep to get a "good" number of threads to use. This gives me on my machine a 25% boost when checking an already cached project.

I also refactored the visitor to use a custom visitor builder.
That allows us to push to local Vec and only merging them when the visitor gets dropped.
I hoped for a more significant speedup than 0-1%. I don't mind reverting that change.

Test Plan

  • ruff-main: Baseline of this PR
  • ruff: This PR
  • ruff-fn: This PR but without using a custom visitor builder
❯ hyperfine --warmup 10 \
        "./target/release/ruff-main check ./crates/ruff_linter/resources/test/cpython/ -e -s" \
        "./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ -e -s" \
       "./target/release/ruff-fn check ./crates/ruff_linter/resources/test/cpython/ -e -s"
Benchmark 1: ./target/release/ruff-main check ./crates/ruff_linter/resources/test/cpython/ -e -s
  Time (mean ± σ):      26.2 ms ±   1.1 ms    [User: 29.2 ms, System: 43.9 ms]
  Range (min … max):    23.5 ms …  28.4 ms    109 runs
 
Benchmark 2: ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ -e -s
  Time (mean ± σ):      21.2 ms ±   1.0 ms    [User: 37.7 ms, System: 58.7 ms]
  Range (min … max):    18.4 ms …  24.6 ms    130 runs
 
Benchmark 3: ./target/release/ruff-fn check ./crates/ruff_linter/resources/test/cpython/ -e -s
  Time (mean ± σ):      21.4 ms ±   0.9 ms    [User: 38.3 ms, System: 58.3 ms]
  Range (min … max):    19.3 ms …  24.4 ms    129 runs
 
Summary
  ./target/release/ruff check ./crates/ruff_linter/resources/test/cpython/ -e -s ran
    1.01 ± 0.06 times faster than ./target/release/ruff-fn check ./crates/ruff_linter/resources/test/cpython/ -e -s
    1.24 ± 0.08 times faster than ./target/release/ruff-main check ./crates/ruff_linter/resources/test/cpython/ -e -s

@MichaReiser MichaReiser added the performance Potential performance improvement label Jul 9, 2024
Comment on lines 382 to 387
builder.threads(
std::thread::available_parallelism()
.map_or(1, std::num::NonZero::get)
.min(12),
);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New default for the number of threads

@MichaReiser MichaReiser force-pushed the reduce-lock-congestion branch from 313f839 to 7a29294 Compare July 9, 2024 13:02
Copy link
Contributor

github-actions bot commented Jul 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet.

@MichaReiser MichaReiser merged commit 4cc7bc9 into main Jul 10, 2024
20 checks passed
@MichaReiser MichaReiser deleted the reduce-lock-congestion branch July 10, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants