Repository navigation
Investigate performance regression for 100,000s of violations #4606
Description
Activity
What changed is that we now always include the source code with the
Diagnostic, regardless ifsilentis specified. This can have a significant cost if there are many diagnostics.I remember the previous version had a fast-path for when silent is enabled.
I think the solution to this is to make
source_codeonMessageoptional as described in #4571. We could then omit the source file when--silenthas been passed.Nit: I recommend using ruff's
-eflag so that hyperfine--ignore-failuresargument isn't necessary. It prevents that you profile failing ruff runs.For my understanding: why does including the source code come with a cost? Isn't it just refcounted?
For my understanding: why does including the source code come with a cost? Isn't it just refcounted?
The cost comes from cloning the
strto aStringonce for every file. I'm not 100% sure if that's the only reason for the regression but it probably adds quiet a bit of overheadBut the number of files for which we're generating violations doesn't change between these rule sets. I just want to sanity check that we're not doing something unintentional like cloning the
strtoStringfor every _diagnostic.Probably relevant:
Diagnostic printing is probably where you want to look at.
--silentshould mean that we're not printing diagnostics at all 😂Here's a flamegraph running the above on main (without hyperfine). You can see that ruff spends the most time in the first thread, sorting diagnostics.
This is a profile from before the PR, it doesn't show the long sorting at the end of the analysis on the main thread.
Edit: I investigated this more, and the culprit is that I included the
diagnostic.kind.rule()in the comparison because I noticed that we have non-deterministic ordering when multiple rules emit a diagnostic in the same file and position.Removing the rule comparison cuts the runtime down from 1s to 320ms
I think there are two cheaper fixes:
- Use
sortinstead ofsort_unstableso that we maintain the order in which the diagnostics were generated. This should be deterministic - And / or: Use
diagnostic.range().ordering(other.range())to also sort by the diagnostic's end position
Reacted by Boshen- Use
I'll follow up with a pr. i have an idea how we could speed this up even more
As of #3931, most rule categories (on the CPython benchmark) got faster, but a few got slower, especially D, ANN, PT, and Q. So running
cargo build --release && hyperfine --ignore-failure --warmup 10 "./target/release/ruff ./crates/ruff/resources/test/cpython/ --no-cache --silent --select D,ANN,PT,Q"went from ~436ms to ~2s on my machine.I think the categories themselves are irrelevant -- it just turns out that these categories have many, many violations (e.g.,
Q000has 143,807 violations).The inverse (
--select ALL --ignore D,ANN,PT,Q) went from 452ms to 764ms, despite the fact that running any of the categories individually got faster.Though most projects (and most Ruff invocations) don't have hundreds of thousands of violations :) it'd be good to understand where this slowdown is coming from. We're running with
--no-cacheand--silent, so we shouldn't be serializing the diagnostics in any way.