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 for detecting function components #3265

Merged

Conversation

golopot
Copy link
Contributor

@golopot golopot commented Apr 6, 2022

Benchmark on a private repo with time npx eslint . (real time):

before after
run 1 6.6s 4.8s
run 2 6.5s 4.7s
run 3 6.7s 4.8s

Benchmark with TIMING=10 npx eslint .:

Before:

Rule                                | Time (ms) | Relative
:-----------------------------------|----------:|--------:
react/destructuring-assignment      |   345.923 |     7.0%
react/no-unstable-nested-components |   208.838 |     4.2%
react/no-did-mount-set-state        |   189.097 |     3.8%
react/boolean-prop-naming           |   187.663 |     3.8%
react/no-direct-mutation-state      |   186.664 |     3.8%
react/no-deprecated                 |   184.020 |     3.7%
react/hook-use-state                |   146.379 |     3.0%
react/no-access-state-in-setstate   |   145.210 |     3.0%
react/prefer-stateless-function     |   133.564 |     2.7%
react/require-optimization          |   128.301 |     2.6%

After:

Rule                                | Time (ms) | Relative
:-----------------------------------|----------:|--------:
react/no-did-mount-set-state        |   181.362 |     6.2%
react/destructuring-assignment      |   162.586 |     5.5%
react/no-deprecated                 |   134.652 |     4.6%
react/no-direct-mutation-state      |   124.110 |     4.2%
react/boolean-prop-naming           |   116.978 |     4.0%
react/no-did-update-set-state       |   113.393 |     3.9%
react/no-unknown-property           |    92.802 |     3.2%
react/hook-use-state                |    91.712 |     3.1%
react/no-unstable-nested-components |    90.477 |     3.1%
react/no-access-state-in-setstate   |    87.266 |     3.0%

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2022

Codecov Report

Merging #3265 (365849c) into master (c297a96) will decrease coverage by 0.00%.
The diff coverage is 98.00%.

@@            Coverage Diff             @@
##           master    #3265      +/-   ##
==========================================
- Coverage   97.69%   97.69%   -0.01%     
==========================================
  Files         121      121              
  Lines        8586     8583       -3     
  Branches     3118     3120       +2     
==========================================
- Hits         8388     8385       -3     
  Misses        198      198              
Impacted Files Coverage Δ
lib/util/jsx.js 95.55% <96.96%> (+1.43%) ⬆️
lib/util/ast.js 95.50% <100.00%> (-0.95%) ⬇️

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 c297a96...365849c. Read the comment docs.

default:
this.skip();
}
},
Copy link
Contributor Author

@golopot golopot Apr 6, 2022

Choose a reason for hiding this comment

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

Avoids unnecessary traversals by skipping all node types but the handled cases.

if (isJSXValue(node)) {
found = true;
breakTraverse();
}
});
Copy link
Contributor Author

@golopot golopot Apr 6, 2022

Choose a reason for hiding this comment

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

Same as above and simplify by not using astUtil.traverse.

Copy link
Member

@ljharb ljharb left a comment

I can't pretend I understand this change, but if all the tests pass, then i guess yay?

tests/util/ast.js Show resolved Hide resolved
@golopot
Copy link
Contributor Author

golopot commented Apr 6, 2022

The gist is that isReturningJSX tries to find return statements that contains JSX. For that a tree traversing is performed. Previously the tree traversing is inefficient because it make unnecessary traversals to nodes that cannot contain a return statement. This PR makes the tree traversal more efficient by only traverse what is needed.

ljharb
ljharb approved these changes Apr 6, 2022
@ljharb ljharb force-pushed the perf-function-component-detection branch from 98c615e to 365849c Compare Apr 6, 2022
@ljharb ljharb merged commit 365849c into jsx-eslint:master Apr 6, 2022
220 checks passed
@golopot golopot deleted the perf-function-component-detection branch Apr 6, 2022
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.

None yet

3 participants