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: don't collect more info than needed when resolving jest functions #1275

Merged
merged 1 commit into from Nov 4, 2022

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Nov 4, 2022

Huge thanks to @SLKnutson for pointing this out - this is mainly a win for larger codebases, since we still have to loop until we find the information we need (or don't), but it is definitely faster

Before, completed in ~30 seconds:

Rule                            | Time (ms) | Relative
:-------------------------------|----------:|--------:
jest/consistent-test-it         |   919.565 |     5.3%
jest/prefer-expect-assertions   |   714.352 |     4.1%
jest/no-conditional-expect      |   626.779 |     3.6%
jest/no-standalone-expect       |   612.223 |     3.5%
jest/prefer-lowercase-title     |   608.857 |     3.5%
jest/no-identical-title         |   599.639 |     3.4%
jest/no-disabled-tests          |   587.099 |     3.4%
jest/prefer-each                |   586.834 |     3.4%
jest/prefer-snapshot-hint       |   585.044 |     3.4%
jest/require-top-level-describe |   579.505 |     3.3%

After, completed in ~15 seconds:

Rule                            | Time (ms) | Relative
:-------------------------------|----------:|--------:
jest/consistent-test-it         |   370.588 |     6.8%
jest/prefer-expect-assertions   |   248.168 |     4.6%
jest/expect-expect              |   232.857 |     4.3%
jest/no-standalone-expect       |   209.712 |     3.9%
jest/no-conditional-expect      |   193.040 |     3.6%
jest/no-disabled-tests          |   173.617 |     3.2%
jest/no-identical-title         |   167.683 |     3.1%
jest/prefer-snapshot-hint       |   161.561 |     3.0%
jest/require-top-level-describe |   159.859 |     3.0%
jest/prefer-each                |   159.230 |     2.9%

Resolves #1274

@SimenB
Copy link
Member

SimenB commented Nov 4, 2022

Woah, surprised by the big difference! Great job 👍

@G-Rath G-Rath marked this pull request as ready for review November 4, 2022 20:26
@SLKnutson
Copy link

Based on percentages, 17.4 seconds vs 5.4 seconds for the actual rules running. The extra time in the full runtime is eslint internals. Nice!

@G-Rath
Copy link
Collaborator Author

G-Rath commented Nov 4, 2022

I just tried the same optimization to scopeHasLocalReference but it doesn't result in any noticeable win so am going to land this as-is; that makes sense anyway because what we're doing there actually requires us most of the time to recuse through everything, and it's only used by two rules 🤷

@G-Rath G-Rath merged commit e4a5674 into main Nov 4, 2022
@G-Rath G-Rath deleted the improve-perf branch November 4, 2022 20:32
github-actions bot pushed a commit that referenced this pull request Nov 4, 2022
## [27.1.4](v27.1.3...v27.1.4) (2022-11-04)

### Performance Improvements

* don't collect more info than needed when resolving jest functions ([#1275](#1275)) ([e4a5674](e4a5674))
@SLKnutson
Copy link

@G-Rath

It might be possible to improve this even more. There is a set field on Scope that I think has a map from variable name => variable, so each level check can use that instead of iterating the variables. When I swapped it out for my project, I got the same result as the existing implementation, but I'm not 100% sure that it works in all scenarios. If it can be used, the performance should be even better. Also, it can probably be used in scopeHasLocalReference too.
Thanks!

const resolveScope = (scope, identifier) => {
  let currentScope = scope;
  while (currentScope !== null) {
    const ref = currentScope.set.get(identifier);
    if (ref && ref.defs.length !== 0) {
      const importDetails = describePossibleImportDef(ref.defs[ref.defs.length - 1]);
      return importDetails?.local === identifier ?  importDetails : 'local';
    }
    currentScope = currentScope.upper;
  }
  return null;
};

@G-Rath
Copy link
Collaborator Author

G-Rath commented Nov 4, 2022

@SLKnutson neat, but for me that doesn't give any noticeable improvements on the performance so I think it's not worth switching just to save a line of code.

@G-Rath
Copy link
Collaborator Author

G-Rath commented Nov 4, 2022

btw @SLKnutson if you wouldn't mind, I would be interested in seeing what the difference is if you are/are not using @jest/globals, since that should trigger slightly different code paths that I think would make a difference. (i.e. if your project is using @jest/globals, switching to using the native globals, and vice versa)

fwiw I've also tried caching resolving things at each level of scope, which also made no significant difference.

@SLKnutson
Copy link

@G-Rath
I haven't tried the global stuff yet, but with the change I mentioned above to resolveScope, I got these speed-ups before/after. Seems like it cuts most rule run times by a factor of 2 or 3, which is noticeable for bigger projects. Thanks!

Rule Time (ms) Relative
jest/consistent-test-it 2892.622 2.6%
jest/no-disabled-tests 1116.793 1.0%
jest/no-identical-title 1051.963 1.0%
jest/no-duplicate-hooks 1045.640 1.0%
jest/require-top-level-describe 1013.668 0.9%
jest/no-alias-methods 599.548 0.5%
jest/prefer-hooks-on-top 558.858 0.5%
jest/valid-title 549.298 0.5%
jest/valid-expect 540.753 0.5%
jest/prefer-comparison-matcher 537.186 0.5%
jest/prefer-to-contain 536.460 0.5%
jest/prefer-todo 533.691 0.5%
jest/no-export 531.140 0.5%
jest/prefer-equality-matcher 522.748 0.5%
jest/no-focused-tests 518.938 0.5%
jest/no-large-snapshots 518.034 0.5%
jest/require-to-throw-message 514.279 0.5%
jest/prefer-to-have-length 513.314 0.5%
jest/valid-describe-callback 512.982 0.5%
jest/no-test-prefixes 511.949 0.5%
jest/prefer-strict-equal 508.050 0.5%
jest/no-interpolation-in-snapshots 507.236 0.5%
jest/prefer-expect-resolves 503.346 0.5%
jest/no-deprecated-functions 268.651 0.2%
jest/no-jasmine-globals 268.419 0.2%
jest/max-nested-describe 110.651 0.1%
jest/no-mocks-import 36.196 0.0%
jest/prefer-spy-on 20.564 0.0%
jest/no-commented-out-tests 9.545 0.0%
Rule Time (ms) Relative
jest/consistent-test-it 1511.273 1.5%
jest/no-disabled-tests 362.679 0.4%
jest/no-identical-title 331.576 0.3%
jest/no-duplicate-hooks 308.397 0.3%
jest/require-top-level-describe 291.826 0.3%
jest/no-deprecated-functions 267.655 0.3%
jest/no-jasmine-globals 265.734 0.3%
jest/no-alias-methods 215.248 0.2%
jest/valid-title 194.013 0.2%
jest/prefer-hooks-on-top 179.216 0.2%
jest/valid-expect 178.825 0.2%
jest/prefer-comparison-matcher 176.338 0.2%
jest/prefer-todo 167.468 0.2%
jest/prefer-to-contain 167.344 0.2%
jest/prefer-expect-resolves 158.118 0.2%
jest/no-export 157.577 0.2%
jest/no-interpolation-in-snapshots 157.258 0.2%
jest/no-large-snapshots 156.854 0.2%
jest/prefer-strict-equal 152.639 0.2%
jest/prefer-equality-matcher 151.352 0.2%
jest/valid-describe-callback 150.444 0.1%
jest/require-to-throw-message 148.396 0.1%
jest/no-focused-tests 147.469 0.1%
jest/no-test-prefixes 147.116 0.1%
jest/prefer-to-have-length 143.708 0.1%
jest/max-nested-describe 91.314 0.1%
jest/no-mocks-import 38.051 0.0%
jest/prefer-spy-on 20.127 0.0%
jest/no-commented-out-tests 9.996 0.0%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rules run a lot slower now
3 participants