perf: Reduce unnecessary LinkPicker re-renders #51613
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcalhoun
added
[Type] Performance
|
Size Change: -2 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4cf171a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5765829006
|
dcalhoun
force-pushed
the
perf/reduce-link-picker-re-renders
branch
from
6c1e36e
to
c3fc3de
Compare
dcalhoun
force-pushed
the
perf/reduce-link-picker-re-renders
branch
from
c3fc3de
to
c438b77
Compare
Combining state with an object in a single `useState` meant a new object was set to the state regardless of whether new values were present. This resulted in unexpected `act` warnings caused by unnecessary re-renders. Refactoring LinkPicker to separate the two state values into separate `useState` calls resolves this issue by using primitives that can be compared directly, which results in `useState` not triggering re-renders when the values are the same.
The wrapping anonymous function is unnecessary now that the callback are simply setting a single state value. Also, passing anonymous functions to child components can lead to unnecessary re-renders.
dcalhoun
force-pushed
the
perf/reduce-link-picker-re-renders
branch
from
c438b77
to
4cf171a
Compare
This argument does not exist in the older version of reassure used. Also, the value is already passed as an environment variable.
dcalhoun
commented
Aug 5, 2023
| @@ -138,7 +138,6 @@ describe.each( [ | |||
| ) | |||
| ); | |||
| if ( type === 'core/image' ) { | |||
| // Wait for side effects produced by Clipboard and link suggestions | |||
There was a problem hiding this comment.
This comment was mistakenly left in place after removing the related, now unnecessary act usage.
| @@ -315,7 +315,7 @@ | |||
| "test:native:clean": "jest --clearCache --config test/native/jest.config.js; rm -rf $TMPDIR/jest_*", | |||
| "test:native:debug": "cross-env NODE_ENV=test node --inspect-brk node_modules/.bin/jest --runInBand --verbose --config test/native/jest.config.js", | |||
| "test:native:perf": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure", | |||
| "test:native:perf:baseline": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure --baseline --testMatch \"**/performance/*.native.[jt]s?(x)\"", | |||
| "test:native:perf:baseline": "cross-env TEST_RUNNER_ARGS='--runInBand --config test/native/jest.config.js --testMatch \"**/performance/*.native.[jt]s?(x)\"' reassure --baseline", | |||
There was a problem hiding this comment.
While the testMatch argument is supported in newer versions of reassure, it is not supported in the version we use. Its inclusion also duplicates the flag passed via environment variable on this same line. This was left in place by mistake.
Once we upgrade reassure, it will be useful to replace the environment variable usage with this inline flag that we may override it ad-hoc, e.g. to run a single test file.
derekblank
approved these changes
Aug 7, 2023
Thanks for reviewing and testing! After reflecting on this specific change and test, I'm unsure the performance issue/fix is substantial enough to warrant a performance test, i.e. this performance issue doesn't cause severe degradations and the fix isn't that complex or unusual. However, I feel the test is likely worth retaining as an example while we look to become comfortable authoring this type of automated test. |
I agree. It is nice to demonstrate the elimination of a re-render as a proof-of-concept, and is a good starting place for continuing our explorations on how to create a deeper editor tree when performance testing. Given that, I support your same reasoning for the test's inclusion. 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Reduce unnecessary
LinkPickerre-renders.Why?
Re-renders include a performance cost, which can degrade the user experience.
How?
Combining state with an object in a single
useStatemeant a new objectwas set to the state regardless of whether new values were present. This
resulted in unexpected
actwarnings caused by unnecessary re-renders.Refactoring
LinkPickerto separate the two state values into separateuseStatecalls resolves this issue by using primitives that can becompared directly, which results in
useStatenot triggering re-renderswhen the values are the same.
Testing Instructions
Automated tests should cover these changes fairly well. Here are a few manual test cases.
Add and edit links within rich text content
Add links for Buttons blocks
Add links to Image blocks
Testing Instructions for Keyboard
n/a
Screenshots or screencast
The included performance test showcases the render count reducing from two to one, a 50% decrease.