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: n/prefer-node-protocol could use optimization #404

Open
1 task
benmccann opened this issue Jan 7, 2025 · 2 comments · May be fixed by #406
Open
1 task

perf: n/prefer-node-protocol could use optimization #404

benmccann opened this issue Jan 7, 2025 · 2 comments · May be fixed by #406
Labels
bug enhancement rule:update An update to a current rule

Comments

@benmccann
Copy link

Environment

Node version: 22.11
npm version: pnpm 9.15.3
ESLint version: 9.9.1
eslint-plugin-n version: 17.9.0
Operating System: Linux

What rule do you want to report?

n/prefer-node-protocol

Link to Minimal Reproducible Example

https://github.com/sveltejs/svelte

What did you expect to happen?

If I set TIMING=20 while running eslint, I can see that almost half the time linting the Svelte codebase is spent on this rule

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   715.705 |    43.9%
no-redeclare                                    |   238.347 |    14.6%
@typescript-eslint/prefer-promise-reject-errors |    88.069 |     5.4%
@typescript-eslint/await-thenable               |    46.862 |     2.9%
svelte/no-unknown-style-directive-property      |    35.708 |     2.2%
no-regex-spaces                                 |    29.598 |     1.8%
@stylistic/quote-props                          |    28.884 |     1.8%
no-global-assign                                |    25.165 |     1.5%
no-misleading-character-class                   |    21.627 |     1.3%
lube/svelte-naming-convention                   |    21.180 |     1.3%
constructor-super                               |    19.286 |     1.2%
no-loss-of-precision                            |    17.931 |     1.1%
no-useless-escape                               |    16.372 |     1.0%
getter-return                                   |    15.868 |     1.0%
no-fallthrough                                  |    15.720 |     1.0%
no-unreachable                                  |    15.522 |     1.0%
no-this-before-super                            |    14.647 |     0.9%
no-control-regex                                |    13.997 |     0.9%
no-dupe-else-if                                 |    12.876 |     0.8%
no-useless-backreference                        |    12.020 |     0.7%

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@benmccann benmccann added the bug label Jan 7, 2025
@scagood scagood added enhancement rule:update An update to a current rule labels Jan 7, 2025
@voxpelli
Copy link
Member

voxpelli commented Jan 7, 2025

I guess some memoization/caching of isEnablingThisRule could help, as it seems to repeat the same version check for every file and doing Denver intersect checks that I guess can be somewhat expensive.

It should at most have to do those checks once for every unique version number.

if (!isEnablingThisRule(context, moduleStyle)) {

@pralkarz
Copy link

I've done some investigation on this topic, and it seems that there are no significant gains from memoizing isEnablingThisRule. I ran ESLint on the Svelte repository with three different implementations twice each (because the variance in absolute values is quite high, not sure whether it's something with my system or a problem in the way ESLint measures performance; when looking at relative values, however, we can see that not much changes).

With eslint-plugin-n from master plugged into Svelte (it's a transitive dependency in that repository, so the currently used version is insignificantly older):

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   902.571 |    51.8%

======================================================================

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   994.851 |    53.8%

With simple Object-based cache:

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   942.266 |    52.6%

======================================================================

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   901.750 |    51.5%

With flru:

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   871.960 |    49.6%

======================================================================

Rule                                            | Time (ms) | Relative
:-----------------------------------------------|----------:|--------:
n/prefer-node-protocol                          |   922.537 |    51.7%

I started digging a little further with Node's perf_hooks. I need to look a bit deeper as I'm unfamiliar with the project, but two examples that stood out are https://github.com/sveltejs/svelte/blob/main/packages/svelte/scripts/generate-version.js with the following results:

PerformanceMeasure {
  name: 'return (visitImport + visitRequire + .reduce)',
  entryType: 'measure',
  startTime: 14681.4016,
  duration: 0.582300000000032,
  detail: 'generate-version.js'
}
PerformanceMeasure {
  name: 'Program:exit',
  entryType: 'measure',
  startTime: 14707.7246,
  duration: 1.0655000000006112,
  detail: 'generate-version.js'
}

And https://github.com/sveltejs/svelte/blob/main/packages/svelte/src/reactivity/date.test.ts where I don't have data for Program:exit (meaning it took less than 0.5 ms which is an arbitrary threshold I set for logging the measurements), but visitImport + visitRequire + .reduce took over 4 ms:

PerformanceMeasure {
  name: 'return (visitImport + visitRequire + .reduce)',
  entryType: 'measure',
  startTime: 15528.6727,
  duration: 4.58520000000135,
  detail: 'date.test.ts'
}

Is there anything that stands out about these files that could contribute to that? Or maybe something that hints at what to look at in the rule itself? Curious to hear your thoughts!

@pralkarz pralkarz linked a pull request Jan 21, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement rule:update An update to a current rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants