-
-
Notifications
You must be signed in to change notification settings - Fork 715
perf(linter): skip rules that do not have any relevant node types #13138
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
perf(linter): skip rules that do not have any relevant node types #13138
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
c7f43f7 to
cff65c0
Compare
8e31c70 to
95f9aa3
Compare
CodSpeed Instrumentation Performance ReportMerging #13138 will improve performances by 4.93%Comparing Summary
Benchmarks breakdown
Footnotes |
fc5effd to
4fe2c46
Compare
95f9aa3 to
3c48f3c
Compare
|
What an unbelievable performance improvement! |
|
Insane! |
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.
Superb!
Probably you intend to do this anyway, but I think need to use syn to parse the rule files. The regex may be, for example, missing the else branch in patterns like this:
if let AstKind::Function(func) = node.kind() {
// ...
} else if let AstKind::ArrowFunctionExpression(arrow) = node.kind() {
// ...
}If it's currently missing out some types, that may slow things down a little.
On the other hand, some patterns aren't currently being caught by the regex, like this one:
let Some(ss) = node.kind().as_switch_statement() else { return };(from no_duplicate_case rule)
Matching those would take more rules out of the "any type" category, and speed things up even more!
I've made a few small comments about the AstTypesIter iterator, but that's just fiddling at the edges.
Edit 7/9/25: This PR has been scaled back a lot from the original. The first version showed ~90% perf improvement. For anyone wondering, that's the reason for all our "amazing perf!" comments.
3c48f3c to
4649cd4
Compare
7d84485 to
3725f46
Compare
4649cd4 to
af12449
Compare
8ffd273 to
37488cd
Compare
af12449 to
f6b3a21
Compare
f2ad2d8 to
48fc2e1
Compare
6ec897e to
b31804a
Compare
Merge activity
|
…3138) - part of #12223 - supersedes stack: #12228 This PR adds the ability to add node type information to indicate what node types a lint rule acts on. The lint rule runner can use this information to optimize running lint rules by skipping nodes, files, or rules that don't apply. This is a simple version that is focused on creating the framework for automatically generating the rule runner code. This is not the end of optimizations. Currently only 102 rules have node type information, while 471 rules have no node type information and will not be skipped. Adding node type information for more rules will improve performance. We may also be able to skip more nodes and files by being clever about how we use this information. For example, filtering out any rules which do not run on any node type, if the file contains no relevant node types for the rule. - Adds a new `RuleRunner` trait which includes information needed for indicating whether a rule has node type info or not - Adds a `oxc_linter_codegen` task which is used to generate the implementation of `RuleRunner` trait for all lint rules. This is mostly written by AI, so worth a bit of skepticism, but I've tried to manually verify the results and it looks good (just lacking more complex detection). - Currently, only rules that contain a single top-level `if` statement (or chain of `else if`), or a single `match` statement will have their node type info generated. This should give us a high level of confidence, since these patterns are relatively easy to verify and identify, but it's only a subset of all rules. Next steps will be to add support for `let...else` - Adjusts the main linting loop to use the node type information to skip rules for node types that don't apply - Updates CI to include a check for any uncommitted linter codegen changes --- ## Benchmarks In the latest version of this PR, CodSpeed shows an up to +13% increase in linter performance across large and small files. In the real-world, I've seen a ~5-15% improvement in performance, but in some cases close to zero for very small files. In either case, there appears to be little downside and a lot of potential upside here. <img width="769" height="242" alt="Screenshot 2025-08-24 at 9 29 30 PM" src="https://github.com/user-attachments/assets/d2c430c4-17a9-4606-85d5-c66c734975d9" /> <img width="757" height="308" alt="Screenshot 2025-08-24 at 9 30 24 PM" src="https://github.com/user-attachments/assets/5508464f-de55-4f4e-9ddd-58f215c4ce85" /> <img width="976" height="368" alt="Screenshot 2025-08-24 at 9 34 52 PM" src="https://github.com/user-attachments/assets/203f83c7-46f3-49f1-a1a8-e18f7ec8483b" />
4f6bc4e to
17c3b21
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
…3138) - part of #12223 - supersedes stack: #12228 This PR adds the ability to add node type information to indicate what node types a lint rule acts on. The lint rule runner can use this information to optimize running lint rules by skipping nodes, files, or rules that don't apply. This is a simple version that is focused on creating the framework for automatically generating the rule runner code. This is not the end of optimizations. Currently only 102 rules have node type information, while 471 rules have no node type information and will not be skipped. Adding node type information for more rules will improve performance. We may also be able to skip more nodes and files by being clever about how we use this information. For example, filtering out any rules which do not run on any node type, if the file contains no relevant node types for the rule. - Adds a new `RuleRunner` trait which includes information needed for indicating whether a rule has node type info or not - Adds a `oxc_linter_codegen` task which is used to generate the implementation of `RuleRunner` trait for all lint rules. This is mostly written by AI, so worth a bit of skepticism, but I've tried to manually verify the results and it looks good (just lacking more complex detection). - Currently, only rules that contain a single top-level `if` statement (or chain of `else if`), or a single `match` statement will have their node type info generated. This should give us a high level of confidence, since these patterns are relatively easy to verify and identify, but it's only a subset of all rules. Next steps will be to add support for `let...else` - Adjusts the main linting loop to use the node type information to skip rules for node types that don't apply - Updates CI to include a check for any uncommitted linter codegen changes --- ## Benchmarks In the latest version of this PR, CodSpeed shows an up to +13% increase in linter performance across large and small files. In the real-world, I've seen a ~5-15% improvement in performance, but in some cases close to zero for very small files. In either case, there appears to be little downside and a lot of potential upside here. <img width="769" height="242" alt="Screenshot 2025-08-24 at 9 29 30 PM" src="https://github.com/user-attachments/assets/d2c430c4-17a9-4606-85d5-c66c734975d9" /> <img width="757" height="308" alt="Screenshot 2025-08-24 at 9 30 24 PM" src="https://github.com/user-attachments/assets/5508464f-de55-4f4e-9ddd-58f215c4ce85" /> <img width="976" height="368" alt="Screenshot 2025-08-24 at 9 34 52 PM" src="https://github.com/user-attachments/assets/203f83c7-46f3-49f1-a1a8-e18f7ec8483b" />
17c3b21 to
a744aff
Compare
Follow-on after #13138. Just code reformatting (inc codegen-generated code).
Follow-on after #13138, as per [this comment](#13138 (comment)). Previously `rules_by_ast_type` and `rules_any_ast_type` contained `&&RuleEnum`, which is unnecessary indirection. Store `&RuleEnum` instead.
Follow-on after #13138, as per [this comment](#13138 (comment)). Pure refactor. Simplify sorting rules, by leaning on derived traits instead of manual implementation.
Follow-on after #13138, as per [this comment](#13138 (comment)). Previously `rules_by_ast_type` and `rules_any_ast_type` contained `&&RuleEnum`, which is unnecessary indirection. Store `&RuleEnum` instead.
Follow-on after #13138, as per [this comment](#13138 (comment)). Pure refactor. Simplify sorting rules, by leaning on derived traits instead of manual implementation.
…3138) - part of #12223 - supersedes stack: #12228 This PR adds the ability to add node type information to indicate what node types a lint rule acts on. The lint rule runner can use this information to optimize running lint rules by skipping nodes, files, or rules that don't apply. This is a simple version that is focused on creating the framework for automatically generating the rule runner code. This is not the end of optimizations. Currently only 102 rules have node type information, while 471 rules have no node type information and will not be skipped. Adding node type information for more rules will improve performance. We may also be able to skip more nodes and files by being clever about how we use this information. For example, filtering out any rules which do not run on any node type, if the file contains no relevant node types for the rule. - Adds a new `RuleRunner` trait which includes information needed for indicating whether a rule has node type info or not - Adds a `oxc_linter_codegen` task which is used to generate the implementation of `RuleRunner` trait for all lint rules. This is mostly written by AI, so worth a bit of skepticism, but I've tried to manually verify the results and it looks good (just lacking more complex detection). - Currently, only rules that contain a single top-level `if` statement (or chain of `else if`), or a single `match` statement will have their node type info generated. This should give us a high level of confidence, since these patterns are relatively easy to verify and identify, but it's only a subset of all rules. Next steps will be to add support for `let...else` - Adjusts the main linting loop to use the node type information to skip rules for node types that don't apply - Updates CI to include a check for any uncommitted linter codegen changes --- ## Benchmarks In the latest version of this PR, CodSpeed shows an up to +13% increase in linter performance across large and small files. In the real-world, I've seen a ~5-15% improvement in performance, but in some cases close to zero for very small files. In either case, there appears to be little downside and a lot of potential upside here. <img width="769" height="242" alt="Screenshot 2025-08-24 at 9 29 30 PM" src="https://github.com/user-attachments/assets/d2c430c4-17a9-4606-85d5-c66c734975d9" /> <img width="757" height="308" alt="Screenshot 2025-08-24 at 9 30 24 PM" src="https://github.com/user-attachments/assets/5508464f-de55-4f4e-9ddd-58f215c4ce85" /> <img width="976" height="368" alt="Screenshot 2025-08-24 at 9 34 52 PM" src="https://github.com/user-attachments/assets/203f83c7-46f3-49f1-a1a8-e18f7ec8483b" />
Follow-on after #13138. Just code reformatting (inc codegen-generated code).
Follow-on after #13138, as per [this comment](#13138 (comment)). Pure refactor. Break out of loop early on first failure, as following iterations have no effect.
Follow-on after #13138. `rules_by_ast_type` has a fixed length, known at compile time. So use a boxed array for it, instead of a `Vec`. This has 2 advantages: 1. `Box<[Vec<T>; N]>` is 8 bytes, instead of 24 for `Vec<Vec<T>>`. 2. The length of the array is encoded in the type, so it may allow compiler to deduce that indexing into it with an `AstType` cannot go out of bounds, and remove bounds checks (maybe - if compiler is smart enough, it may already be able to deduce that with the `Vec`).
## [1.15.0] - 2025-09-11 ### 💥 BREAKING CHANGES - edc70ea allocator/pool: [**BREAKING**] Remove `disable_fixed_size` Cargo feature (#13625) (overlookmotel) ### 🚀 Features - b20b56d linter: Add `vue/no-multiple-slot-args` rule (#13579) (Sysix) - aafe08c linter: Add `vue/define-emits-declaration` rule (#13567) (Sysix) - 2ed5059 linter: Add `vue/define-props-declaration` rule (#13566) (Sysix) - a718c23 linter: Add `vue/valid-define-props` rule (#13565) (Sysix) - 75a673e editor: Support relative path for `oxc.path.server` (#13542) (Sysix) - 4af886b linter: Add `unicorn/no-array-reverse` rule (#13530) (yefan) - 2db32eb data_structures: Add `boxed_slice!` and `boxed_array!` macros (#13596) (overlookmotel) ### 🐛 Bug Fixes - fb9d0f4 language_server: Don't resend diagnostic on save, when `typeAware` is disabled and run is onType (#13604) (YongSeok Jang (장용석)) - 2f36350 editor: Add notice for a possible restart when fixing `filename-case` (#13557) (Sysix) - e17fccc linter: Update `RuleRunner` impl after merge (#13642) (camc314) - 3d27c5b linter/no-unused-private-class-members: False positive with spread expr (#13634) (yefan) - 8314ed5 linter/tsgolint: Correct comment (#13589) (camc314) - 198243b semantic: Dont parse `@` as jsdoc tags inside quotes (#13571) (Gwenn Le Bihan) - 89084d7 linter/custom-plugins: Enforce exact matching for disable directives (#13538) (Copilot) - 277c5e1 linter: Output `eslint-plugin-vue` for vue diagnostics (#13564) (Sysix) - 34d3cde rust: Fix clippy issues (#13540) (Boshen) - 5fccafc linter: `unicorn/prefer-array-flat-map` ignore `React.Children` (#13534) (Sysix) - 7e78e39 linter: Don't panic when parsing regex with multiple parentheses (#13524) (Sysix) - 0d867b1 linter: Skip running tsgolint when no files need type aware linting (#13502) (Copilot) - b677376 language_server: Include the diagnostic of the other linter (#13490) (Sysix) - e87d7bd linter: Parse regex inside `new RegExp()` with parentheses (#13448) (Sysix) - 5990f17 linter: Change `typescript/no-confusing-void-expression` to pedantic (#13473) (Boshen) ### 🚜 Refactor - 7775c21 linter/plugins: Remove `oxlint2` Cargo feature (#13648) (overlookmotel) - 8f37e88 linter: Update tsgolint payload (#13547) (camchenry) - 2d53203 linter/plugins: Move `tokio` usage from `oxc_linter` to `napi/oxlint2` (#13647) (overlookmotel) - 6cd6be2 linter: Add `--experimental-js-plugins` CLI arg (#13658) (overlookmotel) - 476729b linter: Simplify `RuleRunner` trait definition (#13637) (camchenry) - 2f02ac6 linter/plugins: Remove `disable_oxlint2` Cargo feature (#13626) (overlookmotel) - ff9e4fb linter/plugins: Use fixed-size allocators when `ExternalLinter` exists (#13623) (overlookmotel) - f9bff64 linter_codegen: Improve code style for collecting nodes (#13636) (camchenry) - babbaca all: Remove `pub` from modules with no exports (#13618) (overlookmotel) - 91759c6 linter/plugins: Only use `RawTransferFileSystem` if JS plugins registered (#13599) (overlookmotel) - 118020c linter/plugins: Discard `ExternalLinter` if no JS plugins registered (#13598) (overlookmotel) - 8d30bce linter/tsgolint: Report an error if the tsgolint exe could not be found (#13590) (camc314) - bccc276 eslint/for-direction: Clean up implementation and improve documentation (#13532) (Antoine Zanardi) - 1425da2 eslint/default-case-last: Simplify default case last check in switch statement (#13529) (Antoine Zanardi) - d245376 oxlint: Remove unused `runner` module (#13561) (camc314) - 53f2fc1 eslint/default-case: Simplify implementation and enhance readability (#13430) (Antoine Zanardi) - 6f15060 eslint/block-scoped-var: Clean up implementation and improve documentation (#13417) (Antoine Zanardi) - 671e0fd language_server: Only store one instance of a diagnostic (#13514) (Sysix) - 1b425d6 eslint/default-case-last: Simplify implementation and enhance readability (#13515) (Antoine Zanardi) - e4bbbce eslint/default-param-last: Simplify implementation and enhance readability (#13516) (Antoine Zanardi) - e0396fd linter: Remove `static` lifetime from disable directives function argument (#13492) (camc314) ### 📚 Documentation - eb1f167 linter: Note which rules require type info to run on rule page (#13675) (camc314) - e66f93b linter: Fix backtick formatting in no-return-wrap (#13633) (camc314) ### ⚡ Performance - e6a25e7 linter: Remove unnecessary `should_run` check (#13639) (camchenry) - f6a9687 linter: Store rules by AST type in a boxed array (#13578) (overlookmotel) - b81f081 linter: Reduce indirection (#13574) (overlookmotel) - a744aff linter: Skip rules that do not have any relevant node types (#13138) (camchenry) ### 🎨 Styling - e110476 linter: Reformat code (#13573) (overlookmotel) ### 🧪 Testing - 58e6c94 oxlint: Add test for ignorePatterns whitelist (#13372) (Sysix) Co-authored-by: camc314 <18101008+camc314@users.noreply.github.com>

should_runbased on node types (batch 1) #12228This PR adds the ability to add node type information to indicate what node types a lint rule acts on. The lint rule runner can use this information to optimize running lint rules by skipping nodes, files, or rules that don't apply.
This is a simple version that is focused on creating the framework for automatically generating the rule runner code. This is not the end of optimizations. Currently only 102 rules have node type information, while 471 rules have no node type information and will not be skipped. Adding node type information for more rules will improve performance. We may also be able to skip more nodes and files by being clever about how we use this information. For example, filtering out any rules which do not run on any node type, if the file contains no relevant node types for the rule.
RuleRunnertrait which includes information needed for indicating whether a rule has node type info or notoxc_linter_codegentask which is used to generate the implementation ofRuleRunnertrait for all lint rules. This is mostly written by AI, so worth a bit of skepticism, but I've tried to manually verify the results and it looks good (just lacking more complex detection).ifstatement (or chain ofelse if), or a singlematchstatement will have their node type info generated. This should give us a high level of confidence, since these patterns are relatively easy to verify and identify, but it's only a subset of all rules. Next steps will be to add support forlet...elseBenchmarks
In the latest version of this PR, CodSpeed shows an up to +13% increase in linter performance across large and small files. In the real-world, I've seen a ~5-15% improvement in performance, but in some cases close to zero for very small files. In either case, there appears to be little downside and a lot of potential upside here.