perf: fold all late lint passes into one statically-combined pass - #17124
Conversation
234594b to
0e3e477
Compare
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @samueltardieu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
b87cc14 to
08fe0b6
Compare
|
|
|
I've verified the performance improvements, although not as pronounced as in the pull request's description, they're very much appreciable! Thank you for this contribution!, Having so many empty bodies were a pain point and something that I wanted to optimize, but didn't know exactly how. I'll review the PR itself now, and if everything's correct, it'll be merged. |
|
Reminder, once the PR becomes ready for a review, use |
Clippy registers ~300 late passes, each as its own boxed `dyn LateLintPass`. Every `check_*` call is therefore an indirect (vtable) dispatch, even for the many passes that don't override that method and just run the empty default. Walking the HIR multiplies this across every node. Fold all late passes into a single statically-combined struct: one concrete field per pass, with a single `LateLintPass` impl that forwards each `check_*` to every field in turn. Because the field types are concrete and the calls are `#[inline(always)]`, passes that don't override a method contribute a DCE'd empty body and the ones that do become direct, inlined calls. No vtable, no per-node dynamic dispatch, and no monomorphized cons-list to bloat the binary. Each field is wrapped in a `Gated` carrying a precomputed `active` flag derived from the same `lints_that_dont_need_to_run` predicate rustc uses to drop fully-disabled passes, so allow-by-default lints are skipped by a well-predicted branch rather than executed. The `LateLintPass` impl is generated from rustc's `late_lint_methods!` list so the two can never drift. `cargo dev new_lint` now emits a combined-struct field for new late passes at the in-list marker. Measured on an idle machine, same binary, byte-identical diagnostics over a 6-crate corpus (tokio, ripgrep, regex, syn, serde, wasmi): cycles -17% to -28%, instructions -6% to -11%, branch-misses -33% to -65%. lintcheck over the 25-crate default corpus is byte-identical to stock. changelog: none
08fe0b6 to
294e68c
Compare
|
Thank you for the prompt review @blyxyas! I addressed your comment. For the performance improvement, how are you benchmarking? What CPU are you running on? Did the instruction count reduction match? And what was the wall clock time reduction on your machine? I measured on AMD Ryzen 9 9950X3D (Zen 5). Once this is merged, I can send a follow up PR to apply the same approach for the early passes. |
I'm benchmarking via Valgrind, specifically Callgrind testing your commit and the commit prior to your commit. Both built in In ripgrep for example I reported a 8% performance improvement both in icount and in wall time. Which is great! I'll give it a second review |
Clippy registers about 50 early passes, each as its own boxed `dyn EarlyLintPass`. Every `check_*` call is an indirect (vtable) dispatch, even for the many passes that don't override that method and just run the empty default. Walking the AST multiplies this across every node. Fold all early passes into a single statically-combined struct: one concrete field per pass, with a single `EarlyLintPass` impl that forwards each `check_*` to every field in turn. Because the field types are concrete and the calls are `#[inline(always)]`, passes that don't override a method contribute a DCE'd empty body and the ones that do become direct, inlined calls. No vtable and no per-node dynamic dispatch. This mirrors the late-pass combine from rust-lang#17124, but with no `Gated` active flag. rustc drops fully-disabled late passes via `lints_that_dont_need_to_run`, but the early pass runner has no such filtering, so a plain forward is equivalent and a gate would buy nothing. The `EarlyLintPass` impl is generated from rustc's `early_lint_methods!` list so the two cannot drift. `cargo dev new_lint` now emits a combined-struct field for new early passes at the in-list marker. Measured the same way as rust-lang#17124 (valgrind callgrind, release build, this commit against its parent, re-linting the ripgrep 14.1.0 workspace): clippy-driver instructions drop about 3.9%, from 1317445596 to 1266296487. The full UI test suite and dogfood pass unchanged. changelog: none
Awesome work! Only comment I have is, that you should've added a changelog entry. This kind of perf improvement is definitely worth mentioning in our changelog. Maybe do that in the PR for early lint passes. |
…ust-lang#17132) Clippy registers about 50 early passes, each as its own boxed `dyn EarlyLintPass`. Every `check_*` call is an indirect (vtable) dispatch, even for the many passes that don't override that method and just run the empty default. Walking the AST multiplies this across every node. This folds all early passes into a single statically-combined struct: one concrete field per pass, with a single `EarlyLintPass` impl that forwards each `check_*` to every field in turn. Because the field types are concrete and the calls are `#[inline(always)]`, passes that don't override a method contribute a DCE'd empty body and the ones that do become direct, inlined calls. No vtable and no per-node dynamic dispatch. This mirrors the late-pass combine from rust-lang#17124, but with no `Gated` active flag. rustc drops fully-disabled late passes via `lints_that_dont_need_to_run`, but the early pass runner has no such filtering. The `EarlyLintPass` impl is generated from rustc's `early_lint_methods!` list so the two cannot drift. `cargo dev new_lint` now emits a combined-struct field for new early passes at the in-list marker. Measured the same way as rust-lang#17124 (valgrind callgrind, release build, this branch against its parent, re-linting the ripgrep 14.1.0 workspace): clippy-driver instructions drop about 3.9%, from 1317445596 to 1266296487. The improvement is smaller than the late-pass combine because there are far fewer early passes and the AST walk is lighter than the HIR walk. The UI test suite and dogfood pass unchanged. changelog: none
Thanks! The early lint pass was already merged. Is it possible for you to add an entry to the changelog manually? There are a few more PRs with smaller savings that could all be summarized under one changelog item. |
Only if we remember ~10 weeks from now when the changelog is written. Pinging @alex-semenyuk who usually writes the changelog. This will need to be included in the I added the beta-accepted label, since those are checked during changelog writing anyway. But this is for 1.98, not 1.97. |
|
Could you please also update the docs at rust-clippy/book/src/development/defining_lints.md Lines 178 to 198 in 2ba3d1c to reflect this change? Thanks in advance:) |
…ang#17178) Follow up after rust-lang#17124 and rust-lang#17132 changelog: none
Replaces clippy's per-pass late-lint dispatch (one indirect
dynvtable call per registered pass, per HIR node) with a single statically-combined late pass. This means empty default method bodies DCE away, so the surviving overrides become inlined calls behind predicted branches instead of indirect dispatch.Benchmarked with N=10, medians:
I verified diagnostics are identical for all benchmarked crates.
I also adapted
cargo dev new_lintto insert new late passes correctly with the updated syntax.changelog: none