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: switch from object spread to Object.assign when merging globals #16311

Merged
merged 1 commit into from Sep 16, 2022

Conversation

mdjermanovic
Copy link
Member

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:

Refs #16302, improves performance of the new config system.

What changes did you make? (Give an overview)

Replaced object spread with Object.assign when merging globals in Linter.

Performance before the change:

Loading:
  Load performance Run #1:  243.3295ms
  Load performance Run #2:  241.7577ms
  Load performance Run #3:  235.5167ms
  Load performance Run #4:  235.3724ms
  Load performance Run #5:  237.996ms

  Load Performance median:  237.996ms


Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4841.3415ms
  Performance Run #2:  4890.8617ms
  Performance Run #3:  4893.3863ms
  Performance Run #4:  4876.1869ms
  Performance Run #5:  4874.6275000000005ms

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


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  21488.7766ms
  Performance Run #2:  21504.3844ms
  Performance Run #3:  21209.8782ms
  Performance Run #4:  21661.8554ms
  Performance Run #5:  21306.0702ms

  Performance budget exceeded: 21488.7766ms (limit: 14471.243042671615ms)

Performance after the change:

Loading:
  Load performance Run #1:  253.1144ms
  Load performance Run #2:  240.3637ms
  Load performance Run #3:  236.3409ms
  Load performance Run #4:  246.4793ms
  Load performance Run #5:  240.9472ms

  Load Performance median:  240.9472ms


Single File:
  CPU Speed is 2695 with multiplier 13000000
  Performance Run #1:  4928.4451ms
  Performance Run #2:  4910.2437ms
  Performance Run #3:  4891.29ms
  Performance Run #4:  4920.5213ms
  Performance Run #5:  4845.6497ms

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


Multi Files (450 files):
  CPU Speed is 2695 with multiplier 39000000
  Performance Run #1:  13381.7765ms
  Performance Run #2:  13581.8668ms
  Performance Run #3:  13452.5753ms
  Performance Run #4:  13381.2314ms
  Performance Run #5:  13594.2325ms

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

Note: Before running performance tests, I added browser globals in the config:

-"languageOptions": {sourceType: "commonjs"},
+"languageOptions": {sourceType: "commonjs", globals: { ...require("globals").browser } },

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

@eslint-github-bot eslint-github-bot bot added the chore This change is not user-facing label Sep 15, 2022
@netlify
Copy link

netlify bot commented Sep 15, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 0f9c1e8
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6322f5fab5f19e0009a40b86

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Sep 15, 2022
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM.

@snitin315 snitin315 added 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 16, 2022
@mdjermanovic mdjermanovic merged commit 504fe59 into main Sep 16, 2022
@mdjermanovic mdjermanovic deleted the perf-object-assign-globals branch September 16, 2022 22:12
@alfaproject
Copy link

Am I reading your numbers wrongly or the times got worse after the change?

@mdjermanovic
Copy link
Member Author

@alfaproject in these tests, less ms is better. Linting a test project that has 450 files (the Multi Files test) takes 21488.7766ms before the change, 13452.5753ms after the change, so it works faster after the change.

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>
@alfaproject
Copy link

Ah sorry, I didn't realise there was a multi files test. Only noticed the single ones, never mind then

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 16, 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 16, 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 chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants