Skip to content
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

linter: duplicate work in Jest/Vitest rules #6038

Closed
DonIsaac opened this issue Sep 24, 2024 · 2 comments · Fixed by #6722
Closed

linter: duplicate work in Jest/Vitest rules #6038

DonIsaac opened this issue Sep 24, 2024 · 2 comments · Fixed by #6722
Assignees
Labels
A-linter Area - Linter C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance

Comments

@DonIsaac
Copy link
Contributor

DonIsaac commented Sep 24, 2024

This is a big one. I haven't taken the time to write this down until now.

Problem

Most (if not all) eslint-plugin-jest rules use collect_possible_jest_call_node in a run_once block, then pass those nodes to an internal "run" implementation. For example,

// crates/oxc_linter/src/rules/jest/no_hooks.rs
impl Rule for NoHooks {
    fn from_configuration(value: serde_json::Value) -> Self {
        let allow = value
            .get(0)
            .and_then(|config| config.get("allow"))
            .and_then(serde_json::Value::as_array)
            .map(|v| {
                v.iter().filter_map(serde_json::Value::as_str).map(ToString::to_string).collect()
            })
            .unwrap_or_default();

        Self(Box::new(NoHooksConfig { allow }))
    }

    fn run_once(&self, ctx: &LintContext) {
        for possible_jest_node in collect_possible_jest_call_node(ctx) {
            self.run(&possible_jest_node, ctx);
        }
    }
}

This is problematic. Each jest rule repeats the same work, which wastes cycles. To make things worse, collect_possible_jest_call_node allocates nodes it discovers onto the heap.

(Possible) Solution

  1. Update the Rule trait, adding a run_on_jest_node method.
  2. Update Linter::run to use collect_possible_jest_call_node and pass a reference to them to rule.run_on_jest_node
  3. Remove all run_once impls from jest/vitest rules.

Acceptance Criteria

  1. Linter should skip jest node collection if neither the jest or vitest plugins are enabled
  2. Same as above, but non-test files should also be skipped, regardless if jest/vitest plugins are enabled or not

Extra Credit

Bonus points if you can refactor collect_possible_jest_node to return an impl Iterator instead of a Vec.

CC: @camchenry, who has actively been making performance improvements to the linter.

@DonIsaac DonIsaac added C-bug Category - Bug A-linter Area - Linter C-performance Category - Solution not expected to change functional behavior, only performance labels Sep 24, 2024
@camchenry
Copy link
Contributor

I attempted to do this, but I had an absolute heck of a time getting the lifetimes to work out properly. I'm not experienced enough with Rust yet to know exactly where the mistake is, so help appreciated: #6049

@camchenry camchenry self-assigned this Oct 17, 2024
Boshen pushed a commit that referenced this issue Oct 21, 2024
…des (#6721)

- part of #6038

Adds a new `run_on_jest_node` method for running lint rules which allows only Jest/Vitest nodes to be linted. This simplifies a number of Jest rules by removing the need to iterate/collect Jest nodes inside of them. Now, they can simply run on the Jest nodes that are passed to them directly. This also saves time by skipping the running of the rule if it is not a test file or the Jest/Vitest plugins are not enabled.
Boshen pushed a commit that referenced this issue Oct 21, 2024
- closes #6038

Migrates all simple iterations over Jest nodes to use the `run_on_jest_node` method. Rules which require more custom data (such as getting nodes in order or storing data about counts, duplicates, etc.) have not been migrated.

Some simple benchmarking shows that this is ~5% faster on `vscode` when the Jest/Vitest rules are NOT enabled (due to being able to skip them now). And it is up to 8% on `vscode` when the Jest/Vitest plugins are enabled.

```
hyperfine -i -N --warmup 3 --runs 30 './oxlint-jest-iter --deny=all --silent ./vscode' './oxlint-main --deny=all --silent ./vscode'
Benchmark 1: ./oxlint-jest-iter --deny=all --silent ./vscode
  Time (mean ± σ):      2.348 s ±  0.104 s    [User: 16.922 s, System: 0.641 s]
  Range (min … max):    2.141 s …  2.544 s    30 runs

Benchmark 2: ./oxlint-main --deny=all --silent ./vscode
  Time (mean ± σ):      2.476 s ±  0.042 s    [User: 17.768 s, System: 0.668 s]
  Range (min … max):    2.430 s …  2.598 s    30 runs

Summary
  ./oxlint-jest-iter --deny=all --silent ./vscode ran
    1.05 ± 0.05 times faster than ./oxlint-main --deny=all --silent ./vscode
```

```
hyperfine -i -N --warmup 3 --runs 30 './oxlint-jest-iter --deny=all --jest-plugin --vitest-plugin --silent ./vscode' './oxlint-main --deny=all --jest-plugin --vitest-plugin --silent ./vscode'
Benchmark 1: ./oxlint-jest-iter --deny=all --jest-plugin --vitest-plugin --silent ./vscode
  Time (mean ± σ):      2.728 s ±  0.118 s    [User: 19.782 s, System: 0.786 s]
  Range (min … max):    2.580 s …  3.078 s    30 runs

Benchmark 2: ./oxlint-main --deny=all --jest-plugin --vitest-plugin --silent ./vscode
  Time (mean ± σ):      2.939 s ±  0.051 s    [User: 21.259 s, System: 0.859 s]
  Range (min … max):    2.848 s …  3.064 s    30 runs

Summary
  ./oxlint-jest-iter --deny=all --jest-plugin --vitest-plugin --silent ./vscode ran
    1.08 ± 0.05 times faster than ./oxlint-main --deny=all --jest-plugin --vitest-plugin --silent ./vscode
```
@camchenry
Copy link
Contributor

Closing this as done with #6722, we even got the extra credit 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-bug Category - Bug C-performance Category - Solution not expected to change functional behavior, only performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants