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

[new config system] Performance issue #16302

Closed
1 task
zloirock opened this issue Sep 13, 2022 · 8 comments · Fixed by #16339
Closed
1 task

[new config system] Performance issue #16302

zloirock opened this issue Sep 13, 2022 · 8 comments · Fixed by #16339
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:needed
Projects

Comments

@zloirock
Copy link

zloirock commented Sep 13, 2022

Environment

Node version: 18.8.0
npm version: 8.19.1 (local and 8.18.0 global)
Local ESLint version: 8.23.1
Global ESLint version: -
Operating System: macOS 12.6
CPU: M1

What parser are you using?

Default (Espree) + jsonc-eslint-parser

What did you do?

I'm trying to migrate to the flat config.
Old config: https://github.com/zloirock/core-js/blob/master/.eslintrc.js
New config: https://github.com/zloirock/core-js/blob/eslint-flat/eslint.config.js

Just run npm i && npm run lint.

With the old-style config, lining (directly eslint) takes 32 seconds.
With the new-style config, lining (directly eslint) takes 2 minutes and 34 seconds.

What did you expect to happen?

The same (or better) performance.

What actually happened?

5x performance degradation.

Participation

  • I am willing to submit a pull request for this issue.
@zloirock zloirock added bug ESLint is working incorrectly repro:needed labels Sep 13, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 13, 2022
@zloirock
Copy link
Author

Timing with old-style config:

Rule                            | Time (ms) | Relative
:-------------------------------|----------:|--------:
import/no-cycle                 |  1734.228 |    11.7%
indent                          |  1633.629 |    11.0%
import/no-unresolved            |   927.880 |     6.3%
no-redeclare                    |   820.218 |     5.5%
regexp/no-legacy-features       |   574.996 |     3.9%
n/no-deprecated-api             |   480.089 |     3.2%
camelcase                       |   451.625 |     3.0%
import/no-import-module-exports |   400.036 |     2.7%
regexp/confusing-quantifier     |   281.038 |     1.9%
import/no-relative-packages     |   210.507 |     1.4%

Timing with new-style config:

Rule                            | Time (ms) | Relative
:-------------------------------|----------:|--------:
import/no-cycle                 |  2451.559 |    11.4%
indent                          |  2109.449 |     9.8%
no-redeclare                    |  1727.216 |     8.0%
import/no-unresolved            |  1342.104 |     6.3%
import/no-import-module-exports |   795.203 |     3.7%
regexp/no-legacy-features       |   761.688 |     3.5%
n/no-deprecated-api             |   703.821 |     3.3%
camelcase                       |   621.922 |     2.9%
regexp/confusing-quantifier     |   330.971 |     1.5%
import/no-relative-packages     |   272.509 |     1.3%

However, it's not 5x difference.

@mdjermanovic
Copy link
Member

@zloirock thanks for the issue!

I can reproduce a significant difference in performance on my computer. With .eslintrc.js running eslint takes ~1.5 min, while with eslint.config.js it takes ~5 min on the same branch (eslint-flat).

One thing I noticed is that ConfigArray#getConfig takes ~80 sec in total, which seems like a lot. eslintrc's ConfigArray#extractConfig caches configs by the list of matching elements, so maybe we could do the same in the new ConfigArray to improve performance.

Differences in rule timings are surprising as that part of the process is basically the same as with the old config. This will require further analysis.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 15, 2022

This takes more than 2 minutes when linting eslint-flat:

eslint/lib/linter/linter.js

Lines 1604 to 1609 in 8cc0bbe

// add configured globals and language globals
const configuredGlobals = {
...(getGlobalsForEcmaVersion(languageOptions.ecmaVersion)),
...(languageOptions.sourceType === "commonjs" ? globals.commonjs : void 0),
...languageOptions.globals
};

It's merging ~800 custom globals over ~70 builtin globals for each of ~7000 files.

To double-check, I ran this and it's indeed very slow:

const globals = require("globals");

console.log(Object.keys(globals.builtin).length); // 66
console.log(Object.keys(globals.browser).length); // 723

const t = process.hrtime();

for (let i = 0; i < 7000; i++) {
  const obj = {
    ...globals.builtin,
    ...globals.browser
  };
}

console.log(`${process.hrtime(t)[0]}s`); // 152s

This seems to be a problem in v8: https://bugs.chromium.org/p/v8/issues/detail?id=11536

When we replace object spread with Object.assign (#16311), total linting time of eslint-flat goes down from ~300s to ~160s on my computer. It's still more than ~90s with .eslintrc.js. I expect that an improvement in config caching would fix the remaining difference.

@nzakas
Copy link
Member

nzakas commented Sep 21, 2022

One thing I noticed is that ConfigArray#getConfig takes ~80 sec in total, which seems like a lot. eslintrc's ConfigArray#extractConfig caches configs by the list of matching elements, so maybe we could do the same in the new ConfigArray to improve performance.

There is a caching mechanism built into ConfigArray#getConfig. It's cached based on the filename. I don't completely understand the eslintrc caching mechanism, but it looks like it's caching the indices of the elements in the array that match so that it can avoid doing a lot of merges if multiple files match the same elements. That's certainly something that ConfigArray can be made to do.

nzakas added a commit that referenced this issue Sep 21, 2022
Latest version of the package includes more aggressive caching.

Refs #16302
@nzakas
Copy link
Member

nzakas commented Sep 21, 2022

I've just posted #16339 with a new version of config-array. @zloirock can you try this PR with your project to see if it makes any difference? (I'm not seeing a difference in our perf tests but some real world data would be helpful.)

@zloirock
Copy link
Author

zloirock commented Sep 21, 2022

With fixes from above, it takes about 36 seconds - almost the same as with old-style (32 seconds) -)

@mdjermanovic
Copy link
Member

With the performance fixes, I'm getting 89 seconds with eslint.config.js, 90 seconds with .eslintrc.js.

@nzakas
Copy link
Member

nzakas commented Sep 22, 2022

🎉 Awesome!

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Sep 22, 2022
@nzakas nzakas moved this from Feedback Needed to Pull Request Opened in Triage Sep 22, 2022
@nzakas nzakas linked a pull request Sep 22, 2022 that will close this issue
1 task
Triage automation moved this from Pull Request Opened to Complete Sep 22, 2022
mdjermanovic pushed a commit that referenced this issue Sep 22, 2022
Latest version of the package includes more aggressive caching.

Refs #16302
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 22, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:needed
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants