fix(linter): skip per-node dispatch for run_once-only rules in large files#22398
Conversation
…e files The large-files branch (>200k AST nodes) in `execute_rules` added every rule without bucketable AST types to `rules_any_ast_type`, then called `rule.run` per node on the whole bucket — including `run_once`-only rules whose `run` is the default empty impl. With `--debug timings`, each call increments `Calls` and adds `time()` overhead, inflating the "boring rules floor" (unicode-bom, no-dupe-class-members, no-this-before-super, etc.) by N nodes per large file. Gate the else branch on `is_run_implemented()` so `run_once`-only rules are dispatched once via `run_once` and not per node. Verified: a 240k-node file dropped `unicode-bom` from 240,002 calls to 1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
At first glance, this seems like the correct change. Just thinking out loud since it's been quite a few months since I touched the node types optimized code: I'll sleep on it and take another look in the morning, but otherwise I think this seems good to merge. |
|
Also looking at the benchmarks, I see a 14% decrease in self time in 
However, it seems to be eaten up by some other code path that is now getting hit. (Looks like |
|
When benchmarking the release binary from main vs. my "fixed" binary from this branch, sometimes I get a 1.07x, other times it's identical. Not really sure if that means anything, but thus far I have not seen any result that would suggest this is ever less performant, which is good. |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the large-file execution path in oxc_linter by avoiding per-AST-node dispatch for rules that only implement run_once (i.e., rules where run is not implemented). This addresses inflated --debug timings “Calls” and associated overhead for large ASTs (>200k nodes), aligning timing output more closely with actual work performed.
Changes:
- In the large-file branch of
execute_rules, only enqueue rules into the “any AST type” per-node dispatch list whenrunis actually implemented (or when runtime optimization is disabled). - Keeps
run_oncebehavior unchanged:run_once-only rules still execute exactly once per file (as intended), while avoiding N-noderuncalls with a default empty implementation.
The minimum time is consistently decreased, and the slowest run with the optimized code is still faster than the slowest run of the non-optimized code. So even without computing any statistical significance, that seems like a strong signal this is worthwhile to me. After thinking on it, I believe this is the correct behavior. I can't imagine any regressions this might cause. I'm actually a little surprised we didn't catch this earlier! So thank you for reporting this. |
Merge activity
|
# Oxlint ### 🚀 Features - 5478fb5 linter/jsdoc: Implement `require-throws-description` rule (#22386) (Mikhail Baev) - b46d4de linter: Add `--debug` options and add per-rule timing info (#22282) (camchenry) - c73225e linter/eslint: Implement `prefer-arrow-callback` rule (#22312) (박천(Cheon Park)) - de82b59 linter: Add support for `eslint-plugin-jsx-a11y-x` (#22356) (mehm8128) - b170da3 linter: Implement no-implicit-globals (#22249) (Jovi De Croock) - f44b6c8 linter: Fill schemas `DummyRuleMap` with built-in rules (#22288) (Sysix) - 5cdb80d linter/jsx-a11y/: Implement no-interactive-element-to-noninteractive-role (#22332) (anarefolio) - 2749422 linter/jsx-a11y: Add no-noninteractive-element-interactions (#22337) (Pablo Tovar) - ba2a1d3 linter/jsdoc: Implement `require-throws-type` rule (#22358) (Mikhail Baev) - d90729d linter/jsx-a11y: Implement control-has-associated-label (#21985) (mehm8128) - 1d04903 linter/jsdoc: Implement `require-yields-type` rule (#22331) (Mikhail Baev) ### 🐛 Bug Fixes - 04c4609 linter/no-nullable-type-assertion-style: Mark as suggestion (#22450) (camc314) - 1c2b7ec linter/no-unused-vars: Handle shadowed self assignments (#22387) (camc314) - 9faa1d5 linter/no-noninteractive-tabindex: Check conditional expressions (#22435) (camc314) - 0854b3a linter/prefer-arrow-callback: Preserve TSX generic arrows in fixer (#22434) (camc314) - 410b814 linter: Supply `source_type` to codegen fixer (#22433) (camc314) - 3c1bb6f linter: Skip per-node dispatch for run_once-only rules in large files (#22398) (Connor Shea) - 5206cde linter/no-unused-vars: Improve type-only rest parameters diagnostic (#22385) (camc314) - c9a22b5 linter/consistent-function-scoping: Allow imported bindings (#22384) (camc314) - c1e966d linter: Report type-only unused parameters in no-unused-vars (#22368) (camchenry) - 4818d98 linter: Check whether path is under root before ignoring it (#20101) (Leonabcd123) - 41fcdcf linter: Fix rule count not including override rules (#19898) (Daniel Osmond) - 59b4f0e linter: Fix 'explicit-module-boundary-types' false positive with 'allowOverloadFunctions' (#22341) (camchenry) ### ⚡ Performance - 6d42395 linter: Narrow no-unsafe-optional-chaining dispatch (#22437) (camchenry) - 08595fb linter: Optimize no-unreachable (#22397) (camchenry) - 3b46a8d linter: Optimize `no-loss-of-precision` (#22395) (camchenry) - b3e2dc9 linter: Optimize `oxc/bad-array-method-on-arguments` (#22393) (camchenry) ### 📚 Documentation - dcbf62c linter: Remove some duplicate spaces (#22359) (camc314) # Oxfmt ### 💥 BREAKING CHANGES - 21bb5d1 oxfmt: [**BREAKING**] Avoid config pre-scan (#22258) (leaysgur) ### 🐛 Bug Fixes - 441d724 oxfmt: Fix "race probe" logic with unit tests (#22378) (leaysgur) - e49ee26 formatter: Respect `singleQuote` for jsdoc `import()` type paths (#22353) (Colin Lienard) - 43b9978 formatter/sort_imports: Treat subpath imports as internal (#22440) (leaysgur) - 7c5cfa0 formatter: Handle jsx trailing comment with parens (#22370) (leaysgur) - ac5f120 formatter: Fix erroneous formatting inside a template literal with parentheses (#22262) (Jovi De Croock) - 3c53a95 formatter/sort_imports: Handle ignore comment as boundary (#22369) (leaysgur) - 4dd83dd oxfmt: Send expandedStates variants as shared refs (#22366) (leaysgur) - 055cc61 formatter: Expand JSX logical chain with leading line comment (#22346) (leaysgur) - 8046222 formatter: Preserve type alias comment break (#22261) (Jovi De Croock) ### ⚡ Performance - 123c493 oxfmt: Reduce more syscalls (#22380) (leaysgur) Co-authored-by: overlookmotel <557937+overlookmotel@users.noreply.github.com>
Fix #22396. Below is Claude's explanation of the fix. I haven't tested this deeply or fully understood the code yet, so it may be fully wrong, but on first look it does appear to solve the call count problem at the very least.
Summary
The large-files branch (>200k AST nodes) in
execute_ruleswas adding every rule without bucketable AST types torules_any_ast_type, then callingrule.runper node on the entire bucket — includingrun_once-only rules whoserunis the default empty impl.With
--debug timings, each of those per-node calls incrementsCallsand addstime()overhead, inflating the "boring rules floor" (unicode-bom, no-dupe-class-members, no-this-before-super, no-redeclare, no-unused-private-class-members, jsx-filename-extension, etc.) by N nodes per large file.This is what makes those rules cluster near a fixed ~11ms / similar call count in
--debug timingsoutput even on codebases where they have almost no real work to do: a handful of large generated/bundled files dominate the count.The fix: gate the else branch on
is_run_implemented()sorun_once-only rules don't get dispatched throughrunper node.Reproduction
Before:
After:
Impact
--debug timingsoutput: significant correction. The rule timings table now reflects real cost.--debug): small win on files >200k nodes from skipping per-node dispatch into emptyrun; unmeasurable on typical files.Test plan
cargo test -p oxc_linter --lib(1119 tests pass)🤖 Generated with Claude Code