perf(linter): Improve performance of eslint/no-loop-func rule.#22476
perf(linter): Improve performance of eslint/no-loop-func rule.#22476connorshea wants to merge 2 commits into
eslint/no-loop-func rule.#22476Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
`no-loop-func` was reported in oxc-project#22471 as the slowest rule on a real project (23.1% of runtime across 54k invocations). The hot path had three issues: 1. The rule ran on every function in every file, even ones with no loops. Add a `should_run` gate via `AstTypesBitset` so files with no loop statements are skipped entirely. 2. `contains_nested_functions` iterated *all* nodes in the file. Walk only the function's descendants instead: `NodeId`s are assigned in DFS pre-order, so descendants live in `(func_id, ...]` and the walk terminates as soon as a span exits the function. 3. `has_unsafe_references` iterated *all* symbols in the file and, for each one, scanned every reference looking for one inside the function. Replace it with a single descendant walk that collects the unique resolved symbols referenced from inside the function, then checks each one once. On a synthetic 52KB file with 200 top-level symbols and 160 functions inside loops, `cargo run --release` of the rule alone goes from ~6.28ms to ~0.24ms per file (~26x). All 1120 existing linter tests pass unchanged.
68d8e0e to
d3c324e
Compare
There was a problem hiding this comment.
Pull request overview
Optimizes eslint/no-loop-func by dispatching on loop statement nodes (so the codegen'd NODE_TYPES bitset short-circuits the rule on files without loops) and replacing whole-program scans with DFS pre-order descendant walks via contiguous NodeId ranges. The hottest path — has_unsafe_references iterating every symbol in the file per function — is replaced by walking only the function's IdentifierReference descendants and deduping via FxHashSet. Benchmarks show ~1.93x overall speedup.
Changes:
- Refactor
Rule::runto fire on loop statements instead of functions; walk descendants of the loop body and process each function whose innermost containing loop is the current one. - Rewrite
has_unsafe_referencesto enumerateIdentifierReferencedescendants of the function and look up resolved symbols, instead of iterating all symbols. - Update
NODE_TYPESin generated rule runner impls to the five loop kinds; add new pass/fail tests and snapshots.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/eslint/no_loop_func.rs | Loop-dispatched run, descendant-walk based check_loop and has_unsafe_references, new tests |
| crates/oxc_linter/src/generated/rule_runner_impls.rs | NODE_TYPES switched from Function/Arrow to the five loop statement kinds |
| crates/oxc_linter/src/snapshots/eslint_no_loop_func.snap | New diagnostics for the added fail cases |
camc314
left a comment
There was a problem hiding this comment.
I know this works, and delivers a nice perf boost, but i'm worried that it just makes the code so much harder to follow and reason about.
Valid! I can strip it down to just the bare minimum change and see if that's better |
d3c324e to
1a0de89
Compare
Add explicit regression coverage for shapes that exercise the descendant walk in the new loop-dispatch model: - Multiple sibling functions in one loop body (every one should fire, not just the first found). - Functions nested inside if/switch/try inside a loop body (descendant walk must reach them). - Function in for-loop update slot (must not fire — body-span filter). - Multi-statement do-while body, where body precedes test in source.
|
I think it's probably best to optimize this rule manually, I'll close this PR |
- supersedes #22476 - closes #22471 Currently the `no-loop-func` rule looks at _all_ function expressions and then checks if they are inside of loops. In addition, some of the helpers iterate over many nodes trying to determine the context. I've optimized this rule to only look at loop nodes first and then find functions inside of them. This still involves a fair amount of traversing the AST, but we can at least guarantee we will never traverse the whole entire thing, which was possible for. In addition, it is now compatible with the linter codegen automatically, so this rule will be skipped in files with no loops (which is probably quite a few). The general flow now is: 1. Find loop nodes 2. Look for functions inside of the loop (AST visitor) 3. Check each function in the loop: - Like before, check for IIFEs and nested functions - Report if unsafe references Running locally on the `actualbudget/actual` repository this improves the performance considerably: - Before: 410ms (for only this rule) - After: 2ms Produced with the help of AI.
Generated with Claude Code, reviewed and tested by me.
This optimizes the rule by reducing the number of files on which the rule gets run by refactoring it to be able to use codegen'd node types to skip the many files that have functions but no loops, and by walking only function descendants.
Benchmark result:
Fix #22471, I assume we may be able to get the time down further from this with further optimization.
Also add a few additional test cases (these pass on main as well so the behavior is unchanged by this refactor).