-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Benchmark all rules #3570
Benchmark all rules #3570
Conversation
ebb6b0f
to
6cd4eea
Compare
6cd4eea
to
3251a52
Compare
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinux
Windows
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're doubling the number of output rows, should we consider reducing the number of files that are included in the benchmark, just to keep it information-dense? (In other words, how much marginal benefit is there right now for each of the four files we're analyzing? Are any of them redundant?)
The output will have 8 rows after merging (the last 4 are only because the benchmark names between main and this branch don't match).
We could. I didn't spend much time picking the files but my thinking was:
We could potentially remove |
3251a52
to
699b5de
Compare
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
)?), | ||
TestCase::normal(TestFile::try_download("numpy/ctypeslib.py", "https://github.com/numpy/numpy/blob/main/numpy/ctypeslib.py")?), | ||
TestCase::normal(TestFile::try_download("numpy/ctypeslib.py", "https://raw.githubusercontent.com/numpy/numpy/e42c9503a14d66adfd41356ef5640c6975c45218/numpy/ctypeslib.py")?), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops... I never verified if it downloads the correct files. The non-raw endpoints return HTML and not python 🤭
@@ -68,6 +69,28 @@ pub struct TestFile { | |||
code: String, | |||
} | |||
|
|||
static TARGET_DIR: once_cell::sync::Lazy<PathBuf> = once_cell::sync::Lazy::new(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes an issue where the benchmarks created a target folder in the ruff_benchmark
directory instead of re-using Cargo's target
directory (copied from criterion)
699b5de
to
a05d519
Compare
a05d519
to
177f2eb
Compare
Summary
This PR adds the new benchmark group
linter/all-rules
(and renames the existing group tolinter/default-rules
).The motivation of benchmarking all rules is new rules are not part of the default-set and are, thus, not benchmarked. This can result in us missing a new rule that regresses the performance for all users enabling it.
Considerations
Why not change the existing benchmark to run all rules: The default rules benchmark allows us to track the performance of Ruff's infrastructure better. The cost of our infrastructure (scope analysis, traversing the tree) is neglectable when running all rules but is more significant when only running some rules. At least now, the cost of running some more benchmarks is "cheap" because the CI job spends most time building the benchmark.