Reduce memory allocation in rules and utils #2976

willheslam · 2021-04-27T02:52:55Z

Hello!
eslint-plugin-react is an amazing plugin - it catches so many problems that I consider it indispensable :)

Recently on a large-ish project I noticed some of the rules running relatively slowly, so I decided to see if I could speed them up.
In the end I was able to find quite a few opportunities to do less work, mainly by avoiding memory allocations.

I'd really appreciate any feedback on better ways to benchmark the plugin, and how safe these optimisations are.
They pass all the tests, though I appreciate it's not always possible to be 100% confident changes are correct, especially given the large number of changes here - luckily most of them are relatively localised.
I've tried to avoid changing the existing logic as much as possible, though sometimes speed has been picked over readability - I'd appreciate any thoughts on that too.
I've tried to make sure the changes will work on node 4, but that probably needs more thorough testing.

There's a list of the changes summarised at the bottom of this message. Thanks!

Benchmark results:

The project I benchmarked this against using cloc registers as about 550 files, with about 50k lines of code in .tsx files all containing React components.
The changes knock off about 70 - 100ms off the slowest rules, roughly 30% faster, with a sample result here:

Rule                                       | Time (ms) | Relative
:------------------------------------------|----------:|--------:
react/jsx-indent                           |   365.723 |     5.8%
react/no-deprecated                        |   296.861 |     4.7%
react/boolean-prop-naming                  |   286.611 |     4.6%
react/destructuring-assignment             |   283.235 |     4.5%
react/no-unstable-nested-components        |   172.198 |     2.7%
react/no-access-state-in-setstate          |   169.871 |     2.7%
react/jsx-no-bind                          |   161.568 |     2.6%
react/no-direct-mutation-state             |   155.857 |     2.5%
react/prefer-stateless-function            |   150.361 |     2.4%
react/display-name                         |   147.024 |     2.3%
react/no-string-refs                       |   144.832 |     2.3%
react/jsx-sort-props                       |   142.020 |     2.3%
react/no-typos                             |   141.018 |     2.2%
react/default-props-match-prop-types       |   140.545 |     2.2%
react/void-dom-elements-no-children        |   139.404 |     2.2%

to

Rule                                       | Time (ms) | Relative
:------------------------------------------|----------:|--------:
react/jsx-indent                           |   264.386 |     6.4%
react/boolean-prop-naming                  |   176.753 |     4.3%
react/destructuring-assignment             |   146.563 |     3.6%
react/jsx-sort-props                       |   142.513 |     3.5%
react/no-access-state-in-setstate          |   117.366 |     2.8%
react/no-deprecated                        |   109.879 |     2.7%
react/no-direct-mutation-state             |   107.110 |     2.6%
react/void-dom-elements-no-children        |    98.467 |     2.4%
react/display-name                         |    97.823 |     2.4%
react/no-unstable-nested-components        |    95.287 |     2.3%
react/jsx-no-bind                          |    94.966 |     2.3%
react/default-props-match-prop-types       |    92.893 |     2.3%
react/sort-comp                            |    91.416 |     2.2%
react/prefer-stateless-function            |    86.666 |     2.1%
react/no-typos                             |    84.784 |     2.1%

I used the following .eslintrc.js:

module.exports = {
  root: true,
  parser: "@typescript-eslint/parser",
  settings: {
    react: {
      pragma: "React",
      fragment: "Fragment",
      version: "detect",
    },
  },
  extends: ["plugin:react/all"],
}

with the versions:

    "eslint": "^7.25.0",
    "eslint-plugin-react": "^7.23.2",
    "@typescript-eslint/eslint-plugin": "^4.22.0",
    "@typescript-eslint/parser": "^4.22.0",

across the .tsx files in the project, 200 times by running:

$ script timings
$ for i in {1..200}; do TIMING=100 npx eslint . --ext tsx -o eslintlog; done

for both 7.23.2 and then with the lib directory swapped in from this branch.

I then averaged all 200 runs in this google sheet:
https://docs.google.com/spreadsheets/d/1Y5QZ_tsAyXrzgw5Rre21Gmzl95jT9irZmxfwSeF1oI0/edit?usp=sharing

Optimisations:

Avoid allocating empty objects or arrays when possible
Combine filter -> forEach chains
Replace Object.keys with values/entries where possible
Extract deeply nested functions to avoid reallocation
Push expensive operations below guards / early returns
Promote shared or mostly-constant conditionals
Swap compound boolean expressions to proriotise cheap short circuiting
Avoid dynamically generating RegExps when possible
Lazily initialise and cache complex objects and necessarily dynamic RegExps
Move expensive and shared calculations to higher scopes
Replace string splits and trims with RegExps
Avoid redundant array concatenations
Split expensive conditional checks across multiple guards
Move expensive booleans into conditional expressions to exploit short circuiting
Swap out filters for finds where possible
Turn arrays into objects/map/dictionary lookups where possible
Swap array function orders where safe and efficient
Cache string transformations

ljharb

It might be easier to review if this was split up into a separate commit for each conceptual kind of change - since some of them are no-brainers (like the Object.keys -> values) and others are more of a tradeoff between clarity and performance.

ljharb · 2021-04-27T03:16:06Z

lib/rules/boolean-prop-naming.js

-      args.filter((arg) => arg.type === 'ObjectExpression').forEach((object) => validatePropNaming(node, object.properties));
+      args.forEach((object) => object.type === 'ObjectExpression'
+        && validatePropNaming(node, object.properties)
+      );


how meaningful is this specific change? the engine should be able to optimize this down to the equivalent for loop.

Good question.
I'm not convinced v8 can optimise a filter and forEach into an equivalent for loop - some (admittedly slapdash and micro) benchmarking with node v14.15.3 iterating many times over an array shows a singular forEach with a built-in conditional as reliably faster than a filter, forEach chain:

$ ITERATIONS=100 SIZE=10000 node arraytest-filter-for-each.js filter, forEach: time taken 58.374 ms $ ITERATIONS=100 SIZE=10000 node arraytest-just-for-each.js just forEach: time taken 43.283 ms $ ITERATIONS=100 SIZE=10000 node arraytest-for-of-loop.js for of loop: time taken 41.101 ms

The singular forEach is consistently faster - depending on the arrangement of the code seems to be around 20 -> 40% faster.
Going with more iterations but a smaller array shows similar results too, just in case:

$ ITERATIONS=10000 SIZE=100 node arraytest-filter-for-each.js filter, forEach: time taken 45.790 ms $ ITERATIONS=10000 SIZE=100 node arraytest-just-for-each.js just forEach: time taken 40.437 ms $ ITERATIONS=10000 SIZE=100 node arraytest-for-of-loop.js for of loop: time taken 36.737 ms

Here's the gist:
https://gist.github.com/willheslam/d45a1d183d5d778d82c5113ffb8a4ab8
(using a naked for loop is faster than all three, but that's probably going too far)

Whether this specific change is meaningful, I don't honestly have concrete answer - it depends on the frequency the function is called, the average size of args (and how many of them fulfil the condition)
and also how much the surrounding environment is generating memory pressure.

Benchmarking each individual change is difficult, given the noise and iteration time and how some effects of memory allocation are only obvious in concert, so as a blanket rule this PR finds any quick, localised opportunities to reduce memory allocation, where the change didn't massively impact the flow of the code.

ljharb · 2021-04-27T03:16:34Z

lib/rules/default-props-match-prop-types.js

@@ -93,11 +94,13 @@ module.exports = {
        const list = components.list();

        // If no defaultProps could be found, we don't report anything.
-        Object.keys(list).filter((component) => list[component].defaultProps).forEach((component) => {


same with this .filter change to an if check inside forEach?

- Avoid creating new strings by replacing string splits and trims with RegExps - Avoid dynamically generating RegExps when possible - Compare some arrays by identity first, before checking join('') string identity

…ing some closures

- Avoid allocations where possible (e.g. concating with an empty list) - Promote some pure functions to module scope - Avoid some wasteful array allocations (filter & length => reduce / some) - Move some calculations behind early returns / guards

codecov-commenter · 2021-05-04T02:50:59Z

Codecov Report

Merging #2976 (232d6a1) into master (1185b37) will increase coverage by 0.00%.
The diff coverage is 98.65%.

@@           Coverage Diff           @@
##           master    #2976   +/-   ##
=======================================
  Coverage   97.59%   97.59%           
=======================================
  Files         110      110           
  Lines        7269     7368   +99     
  Branches     2651     2695   +44     
=======================================
+ Hits         7094     7191   +97     
- Misses        175      177    +2

Impacted Files	Coverage Δ
lib/rules/jsx-indent.js	`97.29% <87.50%> (-0.04%)`	⬇️
lib/rules/void-dom-elements-no-children.js	`95.65% <90.00%> (+0.19%)`	⬆️
lib/rules/no-unknown-property.js	`96.49% <90.90%> (-0.07%)`	⬇️
lib/rules/no-unused-prop-types.js	`95.83% <94.44%> (+2.08%)`	⬆️
lib/util/version.js	`96.96% <96.00%> (-0.79%)`	⬇️
lib/util/Components.js	`98.56% <98.70%> (-0.31%)`	⬇️
lib/rules/boolean-prop-naming.js	`99.28% <100.00%> (+0.01%)`	⬆️
lib/rules/default-props-match-prop-types.js	`100.00% <100.00%> (ø)`
lib/rules/destructuring-assignment.js	`98.86% <100.00%> (ø)`
lib/rules/display-name.js	`98.16% <100.00%> (+0.05%)`	⬆️
... and 36 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 1185b37...232d6a1. Read the comment docs.

- Cache deprecated config - Precompute versions instead of re-derive it for each check - Avoid reallocating some functions - Avoid normalizing already normalized parts

- Combine filter -> forEach chains - Extract deeply nested functions to higher scopes - Promote shared or mostly-constant values and conditionals to higher scopes - Swap compound boolean expressions to exploit short circuiting - Push expensive operations below guards / early returns - Push expensive operations into conditionals to exploit short circuiting - Swap filters for finds when possible - Swap some array function orders, e.g. Filter before reversing arrays

willheslam · 2021-05-04T08:58:11Z

It might be easier to review if this was split up into a separate commit for each conceptual kind of change - since some of them are no-brainers (like the Object.keys -> values) and others are more of a tradeoff between clarity and performance.

Thanks for suggesting this - I've now split this PR into 13 commits, roughly across the kinds of changes made, or, if hard to disentangle, across the individual modules that have been modified.

ljharb · 2021-05-05T20:24:20Z

lib/util/linkComponents.js

-    DEFAULT_LINK_COMPONENTS.concat(settings.linkComponents || [])
+    settings.linkComponents
+      ? DEFAULT_LINK_COMPONENTS.concat(settings.linkComponents)
+      : DEFAULT_LINK_COMPONENTS


repeating DEFAULT_LINK_COMPONENTS seems unfortunate; is creating an empty array really that costly?

If settings.linkComponents is undefined, then not only would we be creating an empty array, but we'd be creating a copy of DEFAULT_LINK_COMPONENTS too.

On the project I'm testing with, it does this once for each file, or about 500 times.

The docs describe setting custom link components via settings.linkComponents
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md#custom-link-components
but I'm working under the assumption this will usually be undefined, thus avoiding two array allocations per file given that's the default case.

Creating 1000 extra arrays is not incredibly slow, all considered, but if 10 or more rules have this behaviour, this can start to add up and start putting some GC pressure on the rest of the program. How much, specifically in this case, is hard to say.

Looking back at this code again, I've realised that context.settings.linkComponents will be the same across all files, and because jsx-no-target-blank never mutates the resulting Map, it can be created just once for all files!

Creating a copy of an array should be basically free.

ljharb · 2021-05-05T20:25:13Z

lib/util/propWrapper.js

-    return name === func || func.property === name;
-  });
+  const propWrapperFunctions = context.settings.propWrapperFunctions;
+  if (propWrapperFunctions && propWrapperFunctions.length > 0) {


is this check needed? calling [].some() immediately returns false.

This is part of the changes designed to avoid allocating new strings where possible - in this case, avoiding splitting name.
Because we now only conditionally create splitName, we need to conditionally run some too - but we get the added bonus of not having to allocate a closure at all.

isPropWrapperFunction then becomes nearly allocationless, which is nice because it's called a fair amount in a lot of different rules.

Looking at this again, I've realised the predicate only references splitName in some very specific circumstances, so we can sometimes avoid splitting it in the first place and produce a more specialised predicate in the other cases.

(Because context.settings.propWrapperFunctions is meant to be relatively static, we could also run the equivalent of some potentially without allocating a closure at all.)

ljharb · 2021-05-05T20:26:19Z

lib/rules/jsx-indent-props.js

-      } else if (useBracket) {
-        line.isUsingOperator = false;
+      } else {
+        const useBracket = /^([ ]|[\t])*[<]/.test(src);


can we make this regex at module level, rather than nesting it deeper? (same with the useOperator regexes)

Good catch - this actually resulted in my learning that, on top of regex literals being faster than dynamic (new RegExp(string)) ones, in ES5, regex literal instances have their own identity, which means caching them at the module level is faster yet again:
https://stackoverflow.com/questions/9750338/dynamic-vs-inline-regexp-performance-in-javascript/32523333

The storedRegExp is about 5 - 20% percent faster across browsers than inlineRegExp
Always create your immutable regexps with literal syntax and cache it if it's to be re-used.

I'm relatively sure that's not unique to ES5, that's eternally been the semantics in JS.

ljharb · 2021-05-05T20:27:15Z

lib/rules/jsx-indent.js

+      let matches;
      if (byLastLine) {
-        src = lines[lines.length - 1];
+        matches = src.match(/.*\n(.*)$/);
      } else {
-        src = lines[0];
+        matches = src.match(/^(.*)\n/);
      }


Suggested change

let matches;

if (byLastLine) {

src = lines[lines.length - 1];

matches = src.match(/.*\n(.*)$/);

} else {

src = lines[0];

matches = src.match(/^(.*)\n/);

}

const matches = byLastLine ? src.match(/.*\n(.*)$/) : src.match(/^(.*)\n/);

ljharb · 2021-05-05T20:28:42Z

lib/rules/jsx-no-useless-fragment.js

-  return text.trim().length === 0;
+  return !/\S/.test(text);


these aren't necessarily always the same thing; trim can be polyfilled to be more correct, but regexes can't be.

Additionally, is this better than /^[^\S]*$/.test(text), which might be clearer?

Good point - it's hard to know for sure if this regex is truly equivalent to trim - a cursory glance at the spec says it removes "WhiteSpace and LineTerminator.".

The nice thing is it doesn't involve creating a new string, and will finish early the moment it finds a non-whitespace character, rather than continuing on trying to remove all the whitespace.

In terms of clarity I'm not sure - I'm happy to go with your suggestion here, but is /^\s*$/ equivalent, i.e. "Match only on whitespace from start to end"?

I guess I'm not convinced that .trim() is meaningfully slower than using a custom regex. Do you have data to support that?

Sorry about the late reply!

I was really convinced a regex would be faster because there'd be less work (stop the moment non-whitespace is found, rather than doing the work of constructing a new string, then checking its length), but after constructing some basic micro benchmarks to check it, they're basically equivalent in speed, and in Chrome 91 + latest V8 sometimes trim was faster.

Thanks for pushing back on this, I'm glad it was double checked - will revert this change.

ljharb · 2021-05-05T20:38:12Z

tests/util/isFirstLetterCapitalized.js

@@ -20,4 +20,11 @@ describe('isFirstLetterCapitalized', () => {
    assert.equal(isFirstLetterCapitalized('IsCapitalized'), true);
    assert.equal(isFirstLetterCapitalized('UPPERCASE'), true);
  });
+
+  it('should return false for non-letters', () => {


you haven't checked non-english language characters. see https://stackoverflow.com/a/38123020/632724

ljharb · 2021-05-05T20:39:03Z

lib/rules/no-typos.js

@@ -69,7 +83,7 @@ module.exports = {
    }

    function checkValidPropType(node) {
-      if (node.name && !PROP_TYPES.some((propTypeName) => propTypeName === node.name)) {
+      if (node.name && !(node.name in PROP_TYPES)) {


why not use Sets here rather than in? also, using in here ensures we'll get false positives for things like toString, or anything else on Object.prototype.

Good catch on false positives with in, I definitely hadn't considered that.

Set looks like a good fit here!

ljharb · 2021-05-05T20:39:25Z

lib/rules/no-unsafe.js

    /**
     * Checks if a passed method is unsafe
     * @param {string} method Life cycle method
     * @returns {boolean} Returns true for unsafe methods, otherwise returns false
     */
    function isUnsafe(method) {
-      const unsafeMethods = getUnsafeMethods();
-      return unsafeMethods.indexOf(method) !== -1;
+      return (method in unsafe);


ljharb · 2021-05-05T20:40:23Z

lib/rules/no-deprecated.js

@@ -141,8 +157,10 @@ module.exports = {
        return moduleName;
      }

+      const matchName = (name) => name === node.init.name;


might as well hoist this to module level

ljharb · 2021-05-05T20:41:04Z

lib/util/version.js

+function isLessThan(a, b) {
+  const higherMajor = a[0] < b[0];
+  if (higherMajor) {
+    return true;
+  }
+  const higherMinor = a[0] === b[0] && a[1] < b[1];
+  if (higherMinor) {
+    return true;
+  }
+  const higherOrEqualPatch = a[0] === b[0]
+    && a[1] === b[1]
+    && a[2] <= b[2];
+  return higherOrEqualPatch;
 }


this logic doesn't seem like it will work for prerelease versions. i think we need to use the semver package for this.

Interesting - it's equivalent (or should be - will double check this) to what was already here - note the removed lines from function test below.

Happy to consider using semver, but I was hoping to keep this PR functionally equivalent and just a pure performance boost - what do you think?

I mean, we could add semver prior to this PR to correct the behavior, if you prefer :-p

AriPerkkio · 2021-05-12T16:24:40Z

lib/rules/no-unstable-nested-components.js

@@ -35,7 +35,7 @@ function generateErrorMessageWithParentName(parentName) {
 * @returns {Boolean}
 */
 function startsWithRender(text) {
-  return (text || '').startsWith('render');
+  return text !== undefined && text.startsWith('render');


I'm specifically interested in changes related to this rule. Thanks for testing the performance @willheslam!

While this improvement is definitely welcome I'm having a hard time believing it actually speeds up this rule by ~40%.
Is it really that slow to create new Strings? This method is not even accessed that often.
Why shouldn't other Strings also be declared once in module scope, e.g. render, CallExpression?

Rule Time (ms) Relative

react/no-unstable-nested-components 172.198 2.7%

to

Rule Time (ms) Relative

react/no-unstable-nested-components 95.287 2.3%

Good point - the performance gains for no-unstabled-nested-components don't come from the change here - it's from the changes in lib/util/Components.js.
As an example, running soley no-unstable-nested-components on a large-ish project results in isES6Component being invoked many times, and each time (without the change) a regular expression is created dynamically via a string, used once, then discarded.

There are lots of other optimisations inside lib/util/Components.js that are contributing to this speed up too - to prove this is the case, I tried running no-unstable-nested-components with just the lib/util/Components.js changes applied, and got a similar speed up as you note above.

Right, that makes perfect sense. Those utilities are perfect spot for optimization as they are being used widely across the code base.
That's impressive improvement! 💯

willheslam

Responded to comments - thanks for these, very good points.

Will fix what has been pointed out and do some more specific benchmarking to double check some changes.

willheslam · 2021-05-17T02:36:00Z

lib/rules/no-unstable-nested-components.js

@@ -35,7 +35,7 @@ function generateErrorMessageWithParentName(parentName) {
 * @returns {Boolean}
 */
 function startsWithRender(text) {
-  return (text || '').startsWith('render');
+  return text !== undefined && text.startsWith('render');


Good point - the performance gains for no-unstabled-nested-components don't come from the change here - it's from the changes in lib/util/Components.js.
As an example, running soley no-unstable-nested-components on a large-ish project results in isES6Component being invoked many times, and each time (without the change) a regular expression is created dynamically via a string, used once, then discarded.

There are lots of other optimisations inside lib/util/Components.js that are contributing to this speed up too - to prove this is the case, I tried running no-unstable-nested-components with just the lib/util/Components.js changes applied, and got a similar speed up as you note above.

willheslam · 2021-05-17T09:15:46Z

lib/util/linkComponents.js

-    DEFAULT_LINK_COMPONENTS.concat(settings.linkComponents || [])
+    settings.linkComponents
+      ? DEFAULT_LINK_COMPONENTS.concat(settings.linkComponents)
+      : DEFAULT_LINK_COMPONENTS


If settings.linkComponents is undefined, then not only would we be creating an empty array, but we'd be creating a copy of DEFAULT_LINK_COMPONENTS too.

On the project I'm testing with, it does this once for each file, or about 500 times.

The docs describe setting custom link components via settings.linkComponents
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md#custom-link-components
but I'm working under the assumption this will usually be undefined, thus avoiding two array allocations per file given that's the default case.

Creating 1000 extra arrays is not incredibly slow, all considered, but if 10 or more rules have this behaviour, this can start to add up and start putting some GC pressure on the rest of the program. How much, specifically in this case, is hard to say.

Looking back at this code again, I've realised that context.settings.linkComponents will be the same across all files, and because jsx-no-target-blank never mutates the resulting Map, it can be created just once for all files!

willheslam · 2021-05-17T09:20:39Z

lib/util/propWrapper.js

-    return name === func || func.property === name;
-  });
+  const propWrapperFunctions = context.settings.propWrapperFunctions;
+  if (propWrapperFunctions && propWrapperFunctions.length > 0) {


This is part of the changes designed to avoid allocating new strings where possible - in this case, avoiding splitting name.
Because we now only conditionally create splitName, we need to conditionally run some too - but we get the added bonus of not having to allocate a closure at all.

isPropWrapperFunction then becomes nearly allocationless, which is nice because it's called a fair amount in a lot of different rules.

Looking at this again, I've realised the predicate only references splitName in some very specific circumstances, so we can sometimes avoid splitting it in the first place and produce a more specialised predicate in the other cases.

(Because context.settings.propWrapperFunctions is meant to be relatively static, we could also run the equivalent of some potentially without allocating a closure at all.)

willheslam · 2021-05-17T10:01:12Z

lib/rules/jsx-indent-props.js

-      } else if (useBracket) {
-        line.isUsingOperator = false;
+      } else {
+        const useBracket = /^([ ]|[\t])*[<]/.test(src);


Good catch - this actually resulted in my learning that, on top of regex literals being faster than dynamic (new RegExp(string)) ones, in ES5, regex literal instances have their own identity, which means caching them at the module level is faster yet again:
https://stackoverflow.com/questions/9750338/dynamic-vs-inline-regexp-performance-in-javascript/32523333

The storedRegExp is about 5 - 20% percent faster across browsers than inlineRegExp
Always create your immutable regexps with literal syntax and cache it if it's to be re-used.

willheslam · 2021-05-18T02:18:49Z

lib/rules/jsx-no-useless-fragment.js

-  return text.trim().length === 0;
+  return !/\S/.test(text);


Good point - it's hard to know for sure if this regex is truly equivalent to trim - a cursory glance at the spec says it removes "WhiteSpace and LineTerminator.".

The nice thing is it doesn't involve creating a new string, and will finish early the moment it finds a non-whitespace character, rather than continuing on trying to remove all the whitespace.

In terms of clarity I'm not sure - I'm happy to go with your suggestion here, but is /^\s*$/ equivalent, i.e. "Match only on whitespace from start to end"?

lib/util/Components.js

willheslam · 2021-05-18T03:15:42Z

lib/util/Components.js

            ) {
-              return true;
+              pragmaLowerCase = pragmaLowerCase || pragma.toLocaleLowerCase();


Good catch, I'll fix this.

willheslam · 2021-05-18T03:20:52Z

lib/util/isFirstLetterCapitalized.js

@@ -9,8 +9,7 @@ function isFirstLetterCapitalized(word) {
  if (!word) {
    return false;
  }
-  const firstLetter = word.charAt(0);
-  return firstLetter.toUpperCase() === firstLetter;
+  return /^[A-Z]/.test(word);


Good catch - I was too dependent on the existing unit tests for this function.
It'd be great if we could avoid doing some string manipulations here, so it could be that we can use the regex as a fast path, but preserve the chatAt & toUpperCase as a slow path.

willheslam · 2021-05-18T03:25:32Z

lib/rules/no-typos.js

@@ -69,7 +83,7 @@ module.exports = {
    }

    function checkValidPropType(node) {
-      if (node.name && !PROP_TYPES.some((propTypeName) => propTypeName === node.name)) {
+      if (node.name && !(node.name in PROP_TYPES)) {


Good catch on false positives with in, I definitely hadn't considered that.

Set looks like a good fit here!

willheslam · 2021-05-18T03:27:10Z

lib/util/version.js

+function isLessThan(a, b) {
+  const higherMajor = a[0] < b[0];
+  if (higherMajor) {
+    return true;
+  }
+  const higherMinor = a[0] === b[0] && a[1] < b[1];
+  if (higherMinor) {
+    return true;
+  }
+  const higherOrEqualPatch = a[0] === b[0]
+    && a[1] === b[1]
+    && a[2] <= b[2];
+  return higherOrEqualPatch;
 }


Interesting - it's equivalent (or should be - will double check this) to what was already here - note the removed lines from function test below.

Happy to consider using semver, but I was hoping to keep this PR functionally equivalent and just a pure performance boost - what do you think?

ljharb · 2021-08-16T22:08:09Z

@willheslam are you interested in continuing this effort? It seems like there's a number of comments to address, and a few potential PRs to land separately, plus this needs a rebase.

willheslam · 2021-08-17T08:41:59Z

@ljharb Sorry for disappearing on this, work + life got in the way and I ran out of time.

Yeah - I've got some time this week so I'll take a look at the changes I've made to address the comments, changes pending, and what the rebase will entail.

I appreciate your following up on this!

willheslam changed the title ~~Reduce the amount of work done in rules and utils~~ Reduce memory allocation in rules and utils Apr 27, 2021

ljharb requested changes Apr 27, 2021

View reviewed changes

willheslam force-pushed the optimisations branch from 3e716eb to 67a484d Compare May 4, 2021 02:28

willheslam added 6 commits May 4, 2021 03:28

Curry read only props reporting function to avoid creating closures

b4cd869

Replace Object.keys with values/entries

152a276

Avoid allocating empty objects or arrays when possible

35a88cb

Speed up string manipulation and testing

69bcb8d

- Avoid creating new strings by replacing string splits and trims with RegExps - Avoid dynamically generating RegExps when possible - Compare some arrays by identity first, before checking join('') string identity

Speed up Components list by avoiding some array allocations and remov…

da9231a

…ing some closures

Speed up Components utils

8eff94d

- Avoid allocations where possible (e.g. concating with an empty list) - Promote some pure functions to module scope - Avoid some wasteful array allocations (filter & length => reduce / some) - Move some calculations behind early returns / guards

willheslam force-pushed the optimisations branch from 67a484d to 249f085 Compare May 4, 2021 02:49

willheslam requested a review from ljharb May 4, 2021 02:50

willheslam added 7 commits May 4, 2021 04:16

Cache lower cased pragma

671341e

Replace isFirstLetterCapitalised with regex check

0f7183e

Replace array traversals with object property lookup

473d7c1

Speed up no-deprecated

ccdd17c

- Cache deprecated config - Precompute versions instead of re-derive it for each check - Avoid reallocating some functions - Avoid normalizing already normalized parts

Preallocate arrays used as logical or equality checks

a2b5327

Avoid recomputing expensive React version check

d16fcb3

willheslam force-pushed the optimisations branch from 249f085 to 232d6a1 Compare May 4, 2021 03:16

ljharb reviewed May 5, 2021

View reviewed changes

AriPerkkio reviewed May 12, 2021

View reviewed changes

willheslam commented May 18, 2021

View reviewed changes

ljharb marked this pull request as draft August 17, 2021 20:18

ljharb force-pushed the master branch from 0d1a62e to f7fe38f Compare July 11, 2022 15:02

ljharb force-pushed the master branch 2 times, most recently from 47b82ea to 58a3c8b Compare November 10, 2022 20:01

ljharb force-pushed the master branch 4 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45

ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19

ljharb force-pushed the master branch from 57bb8d1 to ae64aa8 Compare June 8, 2023 22:37

Reduce memory allocation in rules and utils #2976

Are you sure you want to change the base?

Reduce memory allocation in rules and utils #2976

Conversation

willheslam commented Apr 27, 2021 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willheslam Apr 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2021 • edited

Codecov Report

willheslam commented May 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willheslam May 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willheslam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willheslam May 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Aug 16, 2021

willheslam commented Aug 17, 2021

willheslam commented Apr 27, 2021 •

edited

willheslam Apr 27, 2021 •

edited

codecov-commenter commented May 4, 2021 •

edited

willheslam commented May 4, 2021 •

edited

willheslam May 17, 2021 •

edited

willheslam May 17, 2021 •

edited