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

perf: fix lazy loading of core rules #15606

Merged
merged 2 commits into from
Feb 17, 2022
Merged

perf: fix lazy loading of core rules #15606

merged 2 commits into from
Feb 17, 2022

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 14, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

Performance improvement.

We have lazy-loading-rule-map which prevents loading all core rule modules when they are not necessary, and that part works well.

On the other hand, eslint-all.js loads all rule modules, because it needs meta of rules to filter out deprecated ones.

The problem is that flat-config-array always loads eslint-all.js:

const allConfig = require("../../conf/eslint-all");

so, as a consequence, basically any use of ESLint loads all rules, which we were obviously trying to avoid.

What changes did you make? (Give an overview)

Moved require("../../conf/eslint-all") from the top level of flat-config-array.js to the place where it's needed.

`npm run perf` before this change

> eslint@8.9.0 perf D:\projects\eslint
> node Makefile.js perf


Loading:
  Load performance Run #1:  622.997785ms
  Load performance Run #2:  685.887445ms
  Load performance Run #3:  617.0843ms
  Load performance Run #4:  689.034299ms
  Load performance Run #5:  615.789688ms

  Load Performance median:  622.997785ms


Single File:
  CPU Speed is 2095 with multiplier 13000000
  Performance Run #1:  11956.969383ms
  Performance Run #2:  11742.944261ms
  Performance Run #3:  11807.732589ms
  Performance Run #4:  12022.606126ms
  Performance Run #5:  11804.399046ms

  Performance budget exceeded: 11807.732589ms (limit: 6205.250596658711ms)


Multi Files (450 files):
  CPU Speed is 2095 with multiplier 39000000
  Performance Run #1:  31217.632838ms
  Performance Run #2:  31106.2355ms
  Performance Run #3:  31508.789833ms
  Performance Run #4:  30745.145445ms
  Performance Run #5:  30807.573268ms

  Performance budget exceeded: 31106.2355ms (limit: 18615.751789976133ms)


`npm run perf` after this change

> eslint@8.9.0 perf D:\projects\eslint
> node Makefile.js perf


Loading:
  Load performance Run #1:  313.853045ms
  Load performance Run #2:  306.604384ms
  Load performance Run #3:  344.540608ms
  Load performance Run #4:  304.535642ms
  Load performance Run #5:  306.038938ms

  Load Performance median:  306.604384ms


Single File:
  CPU Speed is 2095 with multiplier 13000000
  Performance Run #1:  11899.784524ms
  Performance Run #2:  11760.932493ms
  Performance Run #3:  11679.374794ms
  Performance Run #4:  11751.555466ms
  Performance Run #5:  12019.626415ms

  Performance budget exceeded: 11760.932493ms (limit: 6205.250596658711ms)


Multi Files (450 files):
  CPU Speed is 2095 with multiplier 39000000
  Performance Run #1:  31008.965526ms
  Performance Run #2:  30747.93309ms
  Performance Run #3:  30830.182326ms
  Performance Run #4:  31287.272178ms
  Performance Run #5:  31279.041194ms

  Performance budget exceeded: 31008.965526ms (limit: 18615.751789976133ms)


This improves Loading performance by 50%.

Single File and Multi File are not relevant for this change, as they run all rules anyways.

Is there anything you'd like reviewers to focus on?

I tried to write a test for this using proxyquire's @global, and eventually managed to make a test that fails without this change, but didn't manage to remove proxies after the test.

Update: I added a test that runs in a child process and seems to be working well. I've verified that the test would fail without the change in flat-config-array.js. I've also verified that the test would fail if eslint-all.js is loaded from cli-engine.js (as in #15602 (comment)).

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion chore This change is not user-facing labels Feb 14, 2022
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Nice catch!

@nzakas nzakas merged commit 39a2fb3 into main Feb 17, 2022
@nzakas nzakas deleted the perf-lazy-loading-rules branch February 17, 2022 01:08
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.9.0` -> `8.10.0`](https://renovatebot.com/diffs/npm/eslint/8.9.0/8.10.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.10.0`](https://github.com/eslint/eslint/releases/v8.10.0)

[Compare Source](eslint/eslint@v8.9.0...v8.10.0)

#### Features

-   [`6e2c325`](eslint/eslint@6e2c325) feat: Add `ignoreOnInitialization` option to no-shadow rule ([#&#8203;14963](eslint/eslint#14963)) (Soufiane Boutahlil)
-   [`115cae5`](eslint/eslint@115cae5) feat: `--debug` prints time it takes to parse a file ([#&#8203;15609](eslint/eslint#15609)) (Bartek Iwańczuk)
-   [`345e70d`](eslint/eslint@345e70d) feat: Add `onlyOneSimpleParam` option to no-confusing-arrow rule ([#&#8203;15566](eslint/eslint#15566)) (Gautam Arora)

#### Bug Fixes

-   [`cdc5802`](eslint/eslint@cdc5802) fix: Avoid `__dirname` for built-in configs ([#&#8203;15616](eslint/eslint#15616)) (DoZerg)
-   [`ee7c5d1`](eslint/eslint@ee7c5d1) fix: false positive in `camelcase` with combined properties ([#&#8203;15581](eslint/eslint#15581)) (Nitin Kumar)

#### Documentation

-   [`1005bd5`](eslint/eslint@1005bd5) docs: update CLA information ([#&#8203;15630](eslint/eslint#15630)) (Nitin Kumar)
-   [`5d65c3b`](eslint/eslint@5d65c3b) docs: Fix typo in `no-irregular-whitespace` ([#&#8203;15634](eslint/eslint#15634)) (Ryota Sekiya)
-   [`b93af98`](eslint/eslint@b93af98) docs: add links between rules about whitespace around block curly braces ([#&#8203;15625](eslint/eslint#15625)) (Milos Djermanovic)
-   [`ebc0460`](eslint/eslint@ebc0460) docs: update babel links ([#&#8203;15624](eslint/eslint#15624)) (Milos Djermanovic)

#### Chores

-   [`7cec74e`](eslint/eslint@7cec74e) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).2.0 ([#&#8203;15648](eslint/eslint#15648)) (Milos Djermanovic)
-   [`11c8580`](eslint/eslint@11c8580) chore: read `ESLINT_MOCHA_TIMEOUT` env var in Makefile.js ([#&#8203;15626](eslint/eslint#15626)) (Piggy)
-   [`bfaa548`](eslint/eslint@bfaa548) test: add integration tests with built-in configs ([#&#8203;15612](eslint/eslint#15612)) (Milos Djermanovic)
-   [`39a2fb3`](eslint/eslint@39a2fb3) perf: fix lazy loading of core rules ([#&#8203;15606](eslint/eslint#15606)) (Milos Djermanovic)
-   [`3fc9196`](eslint/eslint@3fc9196) chore: include `tests/conf` in test runs ([#&#8203;15610](eslint/eslint#15610)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1190
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
srijan-deepsource pushed a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
* perf: fix lazy loading of core rules

* add test
srijan-deepsource added a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 17, 2022
@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 Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants