Skip to content

perf(linter): Improve performance of eslint/no-loop-func rule.#22476

Closed
connorshea wants to merge 2 commits into
oxc-project:mainfrom
connorshea:claude/improve-no-loop-func-perf-p3jIS
Closed

perf(linter): Improve performance of eslint/no-loop-func rule.#22476
connorshea wants to merge 2 commits into
oxc-project:mainfrom
connorshea:claude/improve-no-loop-func-perf-p3jIS

Conversation

@connorshea

@connorshea connorshea commented May 16, 2026

Copy link
Copy Markdown
Member

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:

Variant with loops (1000 files) no loops (1000 files) total
Original (pre-PR) 41.0 ms 27.0 ms 68.0 ms
Optimized 24.3 ms 11.0 ms 35.3 ms
Speedup vs original ~1.69x ~2.45x ~1.93x

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).

@codspeed-hq

codspeed-hq Bot commented May 16, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 4 untouched benchmarks
⏩ 47 skipped benchmarks1


Comparing connorshea:claude/improve-no-loop-func-perf-p3jIS (9905232) with main (8c1da44)

Open in CodSpeed

Footnotes

  1. 47 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

`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.
@connorshea connorshea force-pushed the claude/improve-no-loop-func-perf-p3jIS branch from 68d8e0e to d3c324e Compare May 16, 2026 04:28
@connorshea connorshea marked this pull request as ready for review May 16, 2026 04:28
@connorshea connorshea requested a review from camc314 as a code owner May 16, 2026 04:28
Copilot AI review requested due to automatic review settings May 16, 2026 04:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::run to 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_references to enumerate IdentifierReference descendants of the function and look up resolved symbols, instead of iterating all symbols.
  • Update NODE_TYPES in 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 camc314 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@connorshea

Copy link
Copy Markdown
Member Author

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

@connorshea connorshea force-pushed the claude/improve-no-loop-func-perf-p3jIS branch 2 times, most recently from d3c324e to 1a0de89 Compare May 16, 2026 16:04
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.
@connorshea

Copy link
Copy Markdown
Member Author

I think it's probably best to optimize this rule manually, I'll close this PR

@connorshea connorshea closed this May 16, 2026
graphite-app Bot pushed a commit that referenced this pull request May 17, 2026
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

linter: Speed up no-loop-func rule

4 participants