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

[DevTools] add perf regression test page in shell #25078

Merged
merged 2 commits into from
Jan 4, 2023

Conversation

mondaychen
Copy link
Contributor

Summary

This PR adds a "perf regression tests" page to react-devtools-shell. This page is meant to be used as a performance sanity check we will run whenever we release a new version or finish a major refactor.
Similar to other pages in the shell, this page can load the inline version of devtools and a test react app on the same page. But this page does not load devtools automatically like other pages. Instead, it provides a button that allows us to load devtools on-demand, so that we can easily compare perf numbers without devtools against the numbers with devtools.

image

As a first step, this page currently only contain one test: mount/unmount a large subtree. This is to catch perf issues that devtools can cause on the react applications it's running on, which was once a bug fixed in #24863.
In the future, we plan to add:

  • more test apps covering different scenarios
  • perf numbers within devtools (e.g. initial load)

How did you test this change?

In order to show this test app can actually catch the perf regression it's aiming at, I reverted #24863 locally. Here is the result:

devtools-regression-with-revert.mov

As shown in the video, the time it takes to unmount the large subtree significantly increased after DevTools is loaded.

For comparison, here is how it looks like before the fix was reverted:
image

about the requestAnimationFrame method

For this test, I used requestAnimationFrame to catch the time when render and commit are done. It aligns very well with the numbers reported by Chrome DevTools performance profiling. For example, in one run, the numbers reported by my method are
image
They are very close to the numbers reported by Chrome profiling:
image

image

<Profiler> is not able to catch this issue here.

If you are aware of a better way to do this, please kindly share with me.

Copy link
Contributor

@lunaruan lunaruan left a comment

Choose a reason for hiding this comment

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

Stamping to unblock but If we want to do it like this we probably should write down a test plan somewhere of how to do this

@mondaychen mondaychen merged commit ff9f943 into facebook:main Jan 4, 2023
github-actions bot pushed a commit that referenced this pull request Jan 4, 2023
## Summary

This PR adds a "perf regression tests" page to react-devtools-shell.
This page is meant to be used as a performance sanity check we will run
whenever we release a new version or finish a major refactor.
Similar to other pages in the shell, this page can load the inline
version of devtools and a test react app on the same page. But this page
does not load devtools automatically like other pages. Instead, it
provides a button that allows us to load devtools on-demand, so that we
can easily compare perf numbers without devtools against the numbers
with devtools.

<img width="561" alt="image"
src="https://user-images.githubusercontent.com/1001890/184059633-e4f0852c-8464-4d94-8064-1684eee626f4.png">

As a first step, this page currently only contain one test:
mount/unmount a large subtree. This is to catch perf issues that
devtools can cause on the react applications it's running on, which was
once a bug fixed in #24863.
In the future, we plan to add:
- more test apps covering different scenarios
- perf numbers within devtools (e.g. initial load)

## How did you test this change?

In order to show this test app can actually catch the perf regression
it's aiming at, I reverted #24863 locally. Here is the result:

https://user-images.githubusercontent.com/1001890/184059214-9c9b308c-173b-4dd7-b815-46fbd7067073.mov

As shown in the video, the time it takes to unmount the large subtree
significantly increased after DevTools is loaded.

For comparison, here is how it looks like before the fix was reverted:
<img width="452" alt="image"
src="https://user-images.githubusercontent.com/1001890/184059743-0968bc7d-4ce4-42cd-b04a-f6cbc078d4f4.png">

## about the `requestAnimationFrame` method

For this test, I used `requestAnimationFrame` to catch the time when
render and commit are done. It aligns very well with the numbers
reported by Chrome DevTools performance profiling. For example, in one
run, the numbers reported by my method are
<img width="464" alt="image"
src="https://user-images.githubusercontent.com/1001890/184060228-990a4c75-f594-411a-9f85-fa5532ec8c37.png">
They are very close to the numbers reported by Chrome profiling:
<img width="456" alt="image"
src="https://user-images.githubusercontent.com/1001890/184060355-a15d1ec5-c296-4016-9c83-03e761f387e3.png">

<img width="354" alt="image"
src="https://user-images.githubusercontent.com/1001890/184060375-19029010-3aed-4a23-890e-397cdba86d9e.png">

`<Profiler>` is not able to catch this issue here.

If you are aware of a better way to do this, please kindly share with
me.

DiffTrain build for [ff9f943](ff9f943)
[View git log for this commit](https://github.com/facebook/react/commits/ff9f943741671b6d83d732b2131d3f7e7d3c54c8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants