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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Upgrade @humanwhocodes/config-array for perf #16339

Merged
merged 1 commit into from Sep 22, 2022

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Sep 21, 2022

Latest version of the package includes more aggressive caching.

Fixes #16302

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:

Upgraded dependency

What changes did you make? (Give an overview)

I upgraded @humanwhocodes/config-array to the latest version. This includes a caching change to cache by matching indices, similar to what eslintrc does. In theory this should improve runtime performance, although running it with npm run perf didn't actually seem to improve the numbers, and in fact, may have made them worse. 馃し

Before:

Single File:
  CPU Speed is 2304 with multiplier 13000000
  Performance Run #1:  6113.9516ms
  Performance Run #2:  6133.3029ms
  Performance Run #3:  6176.0898ms
  Performance Run #4:  5952.241ms
  Performance Run #5:  5964.2055ms

  Performance budget exceeded: 6113.9516ms (limit: 5642.361111111111ms)


Multi Files (450 files):
  CPU Speed is 2304 with multiplier 39000000
  Performance Run #1:  15021.9307ms
  Performance Run #2:  17540.9971ms
  Performance Run #3:  17162.8673ms
  Performance Run #4:  16508.3412ms
  Performance Run #5:  17846.8817ms

  Performance budget exceeded: 17162.8673ms (limit: 16927.083333333332ms)

After:

Single File:
  CPU Speed is 2304 with multiplier 13000000
  Performance Run #1:  6251.8269ms
  Performance Run #2:  6340.695ms
  Performance Run #3:  6383.4306ms
  Performance Run #4:  6483.3105ms
  Performance Run #5:  8135.5252ms

  Performance budget exceeded: 6383.4306ms (limit: 5642.361111111111ms)


Multi Files (450 files):
  CPU Speed is 2304 with multiplier 39000000
  Performance Run #1:  18660.2942ms
  Performance Run #2:  19920.5086ms
  Performance Run #3:  21486.0697ms
  Performance Run #4:  18835.6062ms
  Performance Run #5:  19494.3152ms

  Performance budget exceeded: 19494.3152ms (limit: 16927.083333333332ms)

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

Please double-check that config-array changes to see if they are actually matching the eslintrc behavior.

Latest version of the package includes more aggressive caching.

Refs #16302
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon chore This change is not user-facing labels Sep 21, 2022
@netlify
Copy link

netlify bot commented Sep 21, 2022

Deploy Preview for docs-eslint canceled.

Name Link
馃敤 Latest commit c6eff5a
馃攳 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/632b79962970da000881d52e

@nzakas nzakas linked an issue Sep 22, 2022 that may be closed by this pull request
1 task
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 22, 2022

In theory this should improve runtime performance, although running it with npm run perf didn't actually seem to improve the numbers, and in fact, may have made them worse.

I'm getting better multi-file performance after the change.

Before (with @humanwhocodes/config-array v0.10.4):

Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4864.1887ms
  Performance Run #2:  5338.3667ms
  Performance Run #3:  4875.3077ms
  Performance Run #4:  4863.727ms
  Performance Run #5:  4853.5706ms

  Performance budget exceeded: 4864.1887ms (limit: 4823.747680890538ms)


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  12755.044ms
  Performance Run #2:  12391.5217ms
  Performance Run #3:  12405.5728ms
  Performance Run #4:  12359.3816ms
  Performance Run #5:  12362.7924ms

  Performance budget ok:  12391.5217ms (limit: 14471.243042671615ms)

After (with @humanwhocodes/config-array v0.10.5):

Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4838.7403ms
  Performance Run #2:  4860.4875ms
  Performance Run #3:  4855.6663ms
  Performance Run #4:  4855.972ms
  Performance Run #5:  4869.1903ms

  Performance budget exceeded: 4855.972ms (limit: 4823.747680890538ms)


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  11789.7996ms
  Performance Run #2:  12446.8747ms
  Performance Run #3:  11818.3672ms
  Performance Run #4:  11874.392100000001ms
  Performance Run #5:  12008.8462ms

  Performance budget ok:  11874.392100000001ms (limit: 14471.243042671615ms)

Our perf test config is much simpler than the config in #16302 so the numbers don't show how big the improvement really is. I don't see anything in humanwhocodes/config-array@8a7e8ab that could cause worse numbers.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 22, 2022
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 22, 2022

Please double-check that config-array changes to see if they are actually matching the eslintrc behavior

LGTM!

Some possible further improvements:

  • eslintrc uses Minimatch class and keeps its instances, so each pattern is processed only once. The new config array uses minimatch function, which will repeat the new Minimatch step every time.
  • For ignored files, the config value stored in the cache is undefined, but this code will interpret it as a cache miss.

@mdjermanovic mdjermanovic merged commit 131e646 into main Sep 22, 2022
20 checks passed
@mdjermanovic mdjermanovic deleted the flat-config-caching branch Sep 22, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 27, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.23.1...v8.24.0)

#### Features

-   [`1729f9e`](eslint/eslint@1729f9e) feat: account for `sourceType: "commonjs"` in the strict rule ([#&#8203;16308](eslint/eslint#16308)) (Milos Djermanovic)
-   [`b0d72c9`](eslint/eslint@b0d72c9) feat: add rule logical-assignment-operators ([#&#8203;16102](eslint/eslint#16102)) (fnx)
-   [`f02bcd9`](eslint/eslint@f02bcd9) feat: `array-callback-return` support `findLast` and `findLastIndex` ([#&#8203;16314](eslint/eslint#16314)) (Sosuke Suzuki)

#### Documentation

-   [`2c152ff`](eslint/eslint@2c152ff) docs: note false positive `Object.getOwnPropertyNames` in prefer-reflect ([#&#8203;16317](eslint/eslint#16317)) (AnnAngela)
-   [`bf7bd88`](eslint/eslint@bf7bd88) docs: fix warn severity description for new config files ([#&#8203;16324](eslint/eslint#16324)) (Nitin Kumar)
-   [`8cc0bbe`](eslint/eslint@8cc0bbe) docs: use more clean link syntax ([#&#8203;16309](eslint/eslint#16309)) (Percy Ma)
-   [`6ba269e`](eslint/eslint@6ba269e) docs: fix typo ([#&#8203;16288](eslint/eslint#16288)) (jjangga0214)

#### Chores

-   [`131e646`](eslint/eslint@131e646) chore: Upgrade [@&#8203;humanwhocodes/config-array](https://github.com/humanwhocodes/config-array) for perf ([#&#8203;16339](eslint/eslint#16339)) (Nicholas C. Zakas)
-   [`504fe59`](eslint/eslint@504fe59) perf: switch from object spread to `Object.assign` when merging globals ([#&#8203;16311](eslint/eslint#16311)) (Milos Djermanovic)

</details>

---

### Configuration

馃搮 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - 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).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMDIuMSIsInVwZGF0ZWRJblZlciI6IjMyLjIwMi4zIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1560
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[new config system] Performance issue
3 participants