[performance] don't instantiate a new Regex on every word & visitor

sharmilajesupaul commented 3 years ago

@muenzpraeger I know this isn't the PR you asked for, but we've been trying this rule out at Airbnb and the performance is too slow for us to adopt it at the moment. After some investigation we noticed that most of the time is spent in the find loop in validateIfInclusive. Specifically, creating the regex (which this PR tries to address) & doing a regex test/match on every word.

I have some numbers to show you the impact of this change. Note that both tables are the result of running eslint on a project with around ~220 files (our monorepo is ~80,000 lint-able files). And we're grabbing the top 10 slowest rules in the tables below.

Primary branch (without this change):

Rule                                     | Time (ms) | Relative
:----------------------------------------|----------:|--------:
inclusive-language/use-inclusive-words   |  5099.442 |    58.7%
prettier/prettier                        |  1638.094 |    18.9%
import/no-named-as-default               |   575.031 |     6.6%
import/no-extraneous-dependencies        |   242.964 |     2.8%
react/destructuring-assignment           |   100.284 |     1.2%
react/jsx-no-bind                        |    68.038 |     0.8%
react/no-deprecated                      |    64.522 |     0.7%
rulesdir/no-hyperloop-restricted-imports |    58.105 |     0.7%
react/void-dom-elements-no-children      |    46.405 |     0.5%
import/no-named-as-default-member        |    42.151 |     0.5%
✨  Done in 17.45s.

With this change to stop re-instantiating Regex:

Rule                                     | Time (ms) | Relative
:----------------------------------------|----------:|--------:
inclusive-language/use-inclusive-words   |  2242.047 |    36.9%
prettier/prettier                        |  1766.999 |    29.1%
import/no-named-as-default               |   608.474 |    10.0%
import/no-extraneous-dependencies        |   299.712 |     4.9%
react/destructuring-assignment           |   103.340 |     1.7%
react/jsx-no-bind                        |    67.114 |     1.1%
rulesdir/no-hyperloop-restricted-imports |    61.666 |     1.0%
react/no-deprecated                      |    59.973 |     1.0%
react/void-dom-elements-no-children      |    49.458 |     0.8%
import/no-named-as-default-member        |    46.883 |     0.8%
✨  Done in 15.63s.

Performance improves quite dramatically, but it's still not ideal or fast enough for us to adopt it. It would be great if we didn't have to duplicate the value.test for every word on every visitor but I'm not sure what's the best way to achieve that without restructuring the core logic. However this change should help a bit.

Note that none of the functionality of the rule changes in this PR. The change I did make was to stop instantiating a new Regex class instance on every iteration on ever node visit. So I'm relying on the existing tests to validate this.

cc. @lencioni

github-actions[bot] commented 3 years ago

muenzpraeger commented 3 years ago

I know this isn't the PR you asked for - yes. And it's great. I'll merge it in for now, and will take a look at perf. Good homework! ;-)

muenzpraeger commented 3 years ago

A big perf sucker is here in the merge of a custom config. I'll spend some time thinking about that.

muenzpraeger commented 3 years ago

Ok, just published eslint-plugin-inclusive-language@2.1.1-beta.1. Let me know how that changes the timing for you.

lencioni commented 3 years ago

@muenzpraeger It looks like 2.1.1-beta.1 maybe doesn't have the changes from this PR in it? https://unpkg.com/browse/eslint-plugin-inclusive-language@2.1.1-beta.1/lib/rules/use-inclusive-words.js

Can you double check the publish please?

lencioni commented 3 years ago

Although I don't actually see this change in the code, when I tried it out, it seems to be much faster. Not really sure what's up with that :o

Rule                                   | Time (ms) | Relative
:--------------------------------------|----------:|--------:
prettier/prettier                      |  1837.384 |    49.1%
import/no-extraneous-dependencies      |   159.311 |     4.3%
inclusive-language/use-inclusive-words |   143.364 |     3.8%
react/destructuring-assignment         |   114.908 |     3.1%
import/no-unresolved                   |   100.073 |     2.7%
react/no-deprecated                    |    80.460 |     2.2%
import/named                           |    70.194 |     1.9%
rulesdir/no-unsafe                     |    62.857 |     1.7%
react/void-dom-elements-no-children    |    56.084 |     1.5%
no-unused-vars                         |    43.705 |     1.2%
✨  Done in 12.34s.

muenzpraeger commented 3 years ago

Sorry, that was me when playing around with optimizations. I removed by accident in my branch the change. 🤦🏼‍♂️

Check 2.1.1-beta.2 - @sharmilajesupaul's changes give another real good boost! 🚀

muenzpraeger commented 3 years ago

I just published 2.1.1 with both changes.

lencioni commented 3 years ago

Awesome! 2.1.1 is looking great!

Rule                                   | Time (ms) | Relative
:--------------------------------------|----------:|--------:
prettier/prettier                      |  1549.576 |    48.3%
import/no-extraneous-dependencies      |   173.870 |     5.4%
react/destructuring-assignment         |   119.341 |     3.7%
import/no-unresolved                   |    87.676 |     2.7%
inclusive-language/use-inclusive-words |    79.961 |     2.5%
react/no-deprecated                    |    68.684 |     2.1%
import/named                           |    60.780 |     1.9%
rulesdir/no-unsafe                     |    55.030 |     1.7%
react/void-dom-elements-no-children    |    49.991 |     1.6%
no-unused-vars                         |    40.525 |     1.3%
✨  Done in 11.29s.

sharmilajesupaul commented 3 years ago

yay so fast! 😄 thank you!

muenzpraeger / eslint-plugin-inclusive-language

[performance] don't instantiate a new Regex on every word & visitor #31