Rules run a lot slower now

[SLKnutson]

Hi,
I've noticed that recently this plugin has gotten a lot slower. I use many eslint rules, and still this plugin is over 50% of the runtime. It looks like it spends the majority of its time in parseJestFnCall.

Here is a sampling of the output from TIMING=all

jest/consistent-test-it                                    | 12047.834 |     5.5%
jest/no-disabled-tests                                     |  8272.639 |     3.8%
jest/no-duplicate-hooks                                    |  8047.621 |     3.7%
jest/no-identical-title                                    |  8024.471 |     3.7%
jest/require-top-level-describe                            |  7910.195 |     3.6%
jest/no-alias-methods                                      |  4233.073 |     1.9%
jest/no-export                                             |  4037.647 |     1.9%
jest/valid-expect                                          |  4012.385 |     1.8%
jest/valid-title                                           |  4009.165 |     1.8%
jest/prefer-hooks-on-top                                   |  3998.323 |     1.8%
jest/prefer-comparison-matcher                             |  3997.734 |     1.8%
jest/prefer-to-contain                                     |  3995.129 |     1.8%
jest/prefer-todo                                           |  3993.870 |     1.8%
jest/no-large-snapshots                                    |  3992.662 |     1.8%
jest/prefer-equality-matcher                               |  3988.052 |     1.8%
jest/prefer-strict-equal                                   |  3982.349 |     1.8%
jest/no-interpolation-in-snapshots                         |  3981.740 |     1.8%
jest/prefer-expect-resolves                                |  3981.178 |     1.8%
jest/no-focused-tests                                      |  3972.438 |     1.8%
jest/prefer-to-have-length                                 |  3968.205 |     1.8%
jest/require-to-throw-message                              |  3964.580 |     1.8%
jest/no-test-prefixes                                      |  3957.872 |     1.8%
jest/valid-describe-callback                               |  3945.417 |     1.8%

Thanks!

[G-Rath]

Can you provide some more details about your setup, including the version of node, eslint-plugin-jest, OS, etc?

parseJestFnCall definitely has a higher performance cost as its doing a lot more work, but I don't think there's anything that can really be optimized and the rules should still be very fast compared to others (i.e. they're all only less than 4% each in your timings) - the main performance improvement we could make is to cache the results, which was landed in v26.8.3 via #1187.

Also while your numbers are pretty huge compared to what I was getting in #1187, they still look relatively consistent so suspect that's because of an external factor and impacts all your rules.

[SLKnutson]

Hi,
I'm on:
Node 18.11.0
MacOS 13.0
eslint-plugin-jest 27.1.3

I don't think it's likely that it's related to my setup. As an example, if you point it at the typescript compiler source, it pins the cpu to 100% for at least 15 minutes (I cut it off, not sure how much longer it would have taken). I'm not sure how much of that is eslint internals vs this specific plugin, but it seems like a lot. Every time I pause the debugger, it's executing code in collectReferences.

https://github.com/microsoft/TypeScript/blob/main/lib/typescript.js

{
  "root": true,
  "overrides": [
    {
      "parserOptions": {
        "ecmaVersion": 2021,
        "project": ["tsconfig.json"],
        "sourceType": "module"
      },
      "extends": [],
      "plugins": ["jest"],
      "parser": "@typescript-eslint/parser",
      "files": ["*.ts"],
      "rules": {
        "jest/consistent-test-it": ["error", { "fn": "it", "withinDescribe": "it" }],
        "jest/expect-expect": "off",
        "jest/max-nested-describe": "error",
        "jest/no-alias-methods": "error",
        "jest/no-commented-out-tests": "error",
        "jest/no-conditional-expect": "off",
        "jest/no-deprecated-functions": "error",
        "jest/no-disabled-tests": "error",
        "jest/no-done-callback": "off",
        "jest/no-duplicate-hooks": "error",
        "jest/no-export": "error",
        "jest/no-focused-tests": "error",
        "jest/no-hooks": "off",
        "jest/no-identical-title": "error",
        "jest/no-if": "off",
        "jest/no-interpolation-in-snapshots": "error",
        "jest/no-jasmine-globals": "error",
        "jest/no-large-snapshots": "error",
        "jest/no-mocks-import": "error",
        "jest/no-restricted-matchers": "off",
        "jest/no-restricted-jest-methods": "off",
        "jest/no-standalone-expect": "off",
        "jest/no-test-prefixes": "error",
        "jest/no-test-return-statement": "off",
        "jest/prefer-called-with": "off",
        "jest/prefer-comparison-matcher": "error",
        "jest/prefer-equality-matcher": "error",
        "jest/prefer-expect-assertions": "off",
        "jest/prefer-expect-resolves": "error",
        "jest/prefer-hooks-on-top": "error",
        "jest/prefer-lowercase-title": "off",
        "jest/prefer-spy-on": "error",
        "jest/prefer-strict-equal": "error",
        "jest/prefer-to-be": "off",
        "jest/prefer-to-contain": "error",
        "jest/prefer-to-have-length": "error",
        "jest/prefer-todo": "error",
        "jest/require-hook": "off",
        "jest/require-to-throw-message": "error",
        "jest/require-top-level-describe": "error",
        "jest/valid-describe-callback": "error",
        "jest/valid-expect": "error",
        "jest/valid-expect-in-promise": "off",
        "jest/valid-title": "error"
      }
    }
  ]
}

[G-Rath]

For me attempting to lint the TypeScript codebase results in a OOM, so could you try against the jest codebase instead?

For me doing that with this config:

module.exports = {
  parser: '@typescript-eslint/parser',
  parserOptions: {
    sourceType: 'module',
  },
  plugins: ['jest'],
  env: {
    es2020: true,
    'jest/globals': true,
  },

  extends: [
    'plugin:jest/all'
    ],
  rules: {
  'jest/unbound-method': 'off',
    // 'jest/require-hook': 'off',
    // 'jest/prefer-expect-assertions': 'off'
  }
}

with this command:

NODENV_VERSION=18.11.0 TIMING=1 npx eslint --no-eslintrc --config my-eslint-config.cjs . --ext js,ts,tsx,jsx

Gives:

Rule                          | Time (ms) | Relative
:-----------------------------|----------:|--------:
jest/consistent-test-it       |   737.213 |     5.3%
jest/prefer-expect-assertions |   555.661 |     4.0%
jest/no-conditional-expect    |   505.000 |     3.6%
jest/prefer-snapshot-hint     |   485.972 |     3.5%
jest/prefer-hooks-in-order    |   481.469 |     3.4%
jest/no-identical-title       |   479.283 |     3.4%
jest/no-duplicate-hooks       |   476.965 |     3.4%
jest/prefer-lowercase-title   |   474.335 |     3.4%
jest/no-disabled-tests        |   473.454 |     3.4%
jest/no-standalone-expect     |   465.535 |     3.3%

(this is running on Ubuntu 20.04 via WSL on Windows 11)

My results are an order of magnitude faster than what you're reporting but it does depend on the size of the codebase, and that is still slower than what I originally had in #1187 so I'll have a quick look into that; however as I said in my previous comment the work we're doing is critical path stuff that afaik can't be optimized further without reducing accuracy, and it should still be about the same speed as most other plugins/rules.

[G-Rath]

hmm I can't seem to get anywhere close to the sub 50ms results I posted in #1187, but I've done some flamegraphing and re-reviewed the code yet again and confirmed there's no further easy optimization wins; I wish we didn't have to have this performance hit, but overall I think it's reasonable given we're still under 30-60 seconds when running on a codebase the size of Jest.

I'm happy if someone wants to try and dig into this themselves, in case there is an optimization I've missed, and happy to continue exploring a bit further to find out why the numbers you've reported @SLKnutson are in the thousands instead of the hundreds like mine, but otherwise this issue is probably going to be inactionable.

[SLKnutson]

Here is what I got for the jest repo (with some variance):

Rule	Time (ms)	Relative
jest/consistent-test-it	1017.479	5.1%
jest/prefer-expect-assertions	769.230	3.8%
jest/no-conditional-expect	716.974	3.6%
jest/no-disabled-tests	709.858	3.5%
jest/prefer-lowercase-title	695.739	3.5%
jest/no-identical-title	686.342	3.4%
jest/prefer-each	680.233	3.4%
jest/prefer-snapshot-hint	677.161	3.4%
jest/no-duplicate-hooks	675.940	3.4%
jest/no-standalone-expect	670.467	3.3%

I took a quick look at the code. Recursively searching the scope is a heavy operation regardless, but I bet that doing the checks while recursing instead of building up the 2 sets and a map would give a nice performance lift without requiring heavy refactoring.

[G-Rath]

Here is what I got for the jest repo (with some variance):

Cool, that matches what I got so I think you're seeing the expected performance on whatever project your first results were from.

I bet that doing the checks while recursing instead of building up the 2 sets and a map would give a nice performance lift without requiring heavy refactoring

Could you expand on what you mean by that? We might be able to break out of the loop a bit early in some situations, but that would greatly depend on the structure of the code being linted; anything else I expect would require a heavy refactor, and again I want to stress that these times imo are very acceptable - you've said that the rules are slower now compared to before, but not said if that's an actual crippling problem or given any times that indicate it is.

[G-Rath]

Ok I think I've got something😄 will have a PR up shortly, thanks @SLKnutson!

[SLKnutson]

@G-Rath
That was a quick turn around. I think collectReferences is only through scopeHasLocalReference and can be simplified now too because it's only checking for existence.

Thanks!

[G-Rath]

Yeah I was just trying that, but there's very little gain to be had (since its only called by two rules and usually always to always recurse through everything because of what its trying to figure out) so am going to leave that one as is for now.

[SLKnutson]

In my actual project, the eslint-plugin-jest time went from 116 seconds to 15 seconds 👍

[SimenB]

What's that, 7.5x improvement? Nice!