[react-testing] improved performance by making descendants calculations lazy for element children

[melnikov-s]

Description

Fixes (issue #)

N/A

Type of change

I've been looking into a flaky slow test and doing some profiling and noticed that one of the significant bottlenecks is found in react-testing specifically when attempting to flatten a react fiber linked list into an array of elements. It seems like determining the descendants of each child is an exponential time algorithm. This has a pretty big impact on tests with big fiber trees. I've changed the descendants to be lazily evaluated and derived from the children though grabbing the root descendants still goes through the entire tree which is slow. Additionally I hand optimized some code, for example the spread operator when compiled with typescript to es5 into a spread call seems to be a rather big culprit of performance issues in this computational heavy process.

Performing the above optimization made a significant improvement to the test in question (which had a really big fiber tree ~80 in depth) :

Before:

After:

For other tests with smaller fiber trees the improvement is a more modest 10-20%.

Before:

After:

This should improve unit tests times across the board.

Note: A change in descendants order is a breaking one. I sketched out the order in this PR and it seems to be the same as before, but something worth verifying for the reviewers.

• Patch: Bug (non-breaking change which fixes an issue)
• Minor: New feature (non-breaking change which adds functionality)
• Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

[marutypes]

The code changes look good to me and the perf improvements are very exciting, we should definitely test this in web (or another big codebase) so we can be sure it doesn't secretly break anything though.

[marutypes]

+1 for rename, the preact version of the library I did similar renames :)

[marutypes]

I wonder if we could change our compilation settings for this package to have the foreach version of this not get transpiled, this is a node library, and node definitely supports foreach.

[melnikov-s]

I've tried targeting ES2015 and changing that function to a more simplified:

function getDescendants(element: any) {
  const descendants: Element<unknown>[] = [];
  for (const child of element.allChildren) {
    if (typeof child !== 'string') {
      descendants.push(child, ...child.elementDescendants);
    }
  }

  return descendants;
}

but it is unfortunately considerably slower :(

for of with spread operator:

for loop with no spread:

(There's a slight variance between runs. I run these tests several times, wait for them to stabilize and take the lowest run time)

[melnikov-s]

Things are looking stable. I've ran all tsx tests in web and here are the results:

Before: main

After: react-testing-lazy-descendants

The failures are the same for both and unrelated to this change, the run on the main branch has an additional timeout result which is why it has one more failure.

One thing I've noticed is that there's a test Globe.test.tsx that's consistently passing on main and has intermittent failures on this branch. It has something to do with timing as the behavior should be the same between the two branches. I've added an artificial delay to the fiberToElement function and sure enough got the tests to consistently pass. Globe.tsx has a setTimeout(fn, 0) to flip a loading state for whatever reason so I think using jest fake timers and advancing that timer manually should fix the issue. Other then that, it's looking good.

[melnikov-s]

Found another optimization, was looking into why the depth is so large in web and noticed that our main AppContext component is some 25 providers deep. I've added an inner wrapper so that we can get a reference to the root element without first having to traverse and build elements for all the providers which will be thrown away anyway as soon as root is resolved.

Here's the latest run:

Here are some flamegraphs of a single test highlighting the optimized functions

before:

after:

Looks like any significant overhead from react-testing has been eliminated

[atesgoral]

@melnikov-s HUGE!

[marutypes]

@vsumner confirmed with a beta that the lazy evaluation change doesn't cause regressions in web, so that is good by me.

If you like you could pull the new er optimization into another PR and we can test that one with a beta too, and get the original one shipped ASAP

[melnikov-s]

If you like you could pull the new er optimization into another PR and we can test that one with a beta too, and get the original one shipped ASAP

Thanks, #1821

[vsumner]

@melnikov-s could you please add a changelog entry? And then feel free to merge and I can cut a new release.

[melnikov-s]

@melnikov-s could you please add a changelog entry? And then feel free to merge and I can cut a new release.

Will do, is this a breaking change that requires a major version bump? I'm noticing that Element is exported from the module and I've changed it's constructor signature.

[marutypes]

@melnikov-s I don't think its intended that folks manually construct elements, rather they can just use Element for type annotations in stuff like custom matchers if they need to, so my bias would be towards not doing a major bump

that said it is technically correct to do a major one I suppose

[melnikov-s]

@TheMallen Yeah, I assumed it was for types. We might want to do export type { Element }; at some point to solidify that (or if we decide to bump it to major).