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

[Refactor] improve performance by avoiding unnecessary component detect #3273

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Apr 18, 2022

Rules using Component.detect() are generally slow. Some rules uses Component.detect(context, components, utils) solely for the utils it provided. These Component.detects can be avoided by extracting some utils functions into a module.

Benchmark with config plugin:react/all:

$ time npx eslint .
- 16.429
+ 13.692

@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #3273 (cbaf278) into master (2bf54d7) will increase coverage by 0.01%.
The diff coverage is 99.43%.

❗ Current head cbaf278 differs from pull request most recent head 18de0a6. Consider uploading reports for the commit 18de0a6 to get more accurate results

@@            Coverage Diff             @@
##           master    #3273      +/-   ##
==========================================
+ Coverage   97.68%   97.69%   +0.01%     
==========================================
  Files         121      122       +1     
  Lines        8592     8634      +42     
  Branches     3127     3135       +8     
==========================================
+ Hits         8393     8435      +42     
  Misses        199      199              
Impacted Files Coverage Δ
lib/util/pragma.js 93.10% <ø> (ø)
lib/util/componentUtil.js 98.66% <98.66%> (ø)
lib/rules/display-name.js 98.21% <100.00%> (+0.01%) ⬆️
lib/rules/jsx-indent.js 98.31% <100.00%> (-0.01%) ⬇️
lib/rules/jsx-no-bind.js 100.00% <100.00%> (ø)
lib/rules/no-access-state-in-setstate.js 93.75% <100.00%> (+0.07%) ⬆️
lib/rules/no-arrow-function-lifecycle.js 98.36% <100.00%> (+0.02%) ⬆️
lib/rules/no-deprecated.js 98.03% <100.00%> (+0.05%) ⬆️
lib/rules/no-direct-mutation-state.js 98.11% <100.00%> (+0.03%) ⬆️
lib/rules/no-redundant-should-component-update.js 96.00% <100.00%> (+0.16%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bf54d7...18de0a6. Read the comment docs.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Before I review the whole thing - this seems like a very scary change to me. I'd much rather make component detection faster, and have every rule unconditionally be wrapped with component detection logic, than risk false positive warnings just to eke out a bit more performance from an eslint run (something that even without jest-eslint-runner is pretty fast already)

lib/rules/jsx-no-bind.js Show resolved Hide resolved
@golopot
Copy link
Contributor Author

golopot commented Apr 19, 2022

This change does not cause more reports or less reports. This change is a safe refactoring that does not change ouput. The Component.detect thing has no effect if you are not using components or utils it provides. Using Component.detect without using components or utils is analogous to spending lots of time computing a value and not using it.

@ljharb
Copy link
Member

ljharb commented Apr 19, 2022

My expectation would be that using Components.detect would restrict the AST node visitors to only applying within components. If that's not how it works, then I clearly misunderstand the abstraction.

@golopot
Copy link
Contributor Author

golopot commented Apr 19, 2022

My expectation would be that using Components.detect would restrict the AST node visitors to only applying within components. If that's not how it works, then I clearly misunderstand the abstraction.

Yes some rules uses Component.detect((context) => { return { ImportDeclaration(node) {}}) }, event though ImportDeclaration cannot possibly be in a component.

@ljharb
Copy link
Member

ljharb commented Apr 19, 2022

Fair enough; I'll rereview.

lib/types.d.ts Show resolved Hide resolved
lib/util/Components.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants