-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Header toolbar: useCallback to avoid unnecessary rerenders #32406
Conversation
Size Change: +66 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
c1dea65
to
b62b6f4
Compare
@@ -73,6 +76,10 @@ function HeaderToolbar() { | |||
/* translators: accessibility text for the editor toolbar */ | |||
const toolbarAriaLabel = __( 'Document tools' ); | |||
|
|||
const toggleListView = useCallback( | |||
() => setIsListViewOpened( ! isListViewOpen ), | |||
[ setIsListViewOpened, isListViewOpen ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setter and value here need to be dependencies since this is using a data store.
As an aside this is avoidable in similar handlers when using useState. (Eg where a state setter does not change and using a functional update to avoid a closure with stale data. )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding all these unnecessary renders!
Yes, the difference in ergonomics between useState
entities and useSelect/useDispatch
ones is striking. However, as of #31078 we encourage an "advanced" use case of useSelect
that fits the current case very well:
diff --git a/packages/edit-post/src/components/header/header-toolbar/index.js b/packages/edit-post/src/components/header/header-toolbar/index.js
index f3e16a6f2b..1ddab499e2 100644
--- a/packages/edit-post/src/components/header/header-toolbar/index.js
+++ b/packages/edit-post/src/components/header/header-toolbar/index.js
@@ -77,10 +77,10 @@ function HeaderToolbar() {
/* translators: accessibility text for the editor toolbar */
const toolbarAriaLabel = __( 'Document tools' );
- const toggleListView = useCallback(
- () => setIsListViewOpened( ! isListViewOpen ),
- [ setIsListViewOpened, isListViewOpen ]
- );
+ const { isListViewOpened } = useSelect( editPostStore );
+ const toggleListView = useCallback( () => {
+ setIsListViewOpened( ! isListViewOpened() );
+ }, [] );
const overflowItems = (
<>
<ToolbarItem
diff --git a/packages/edit-site/src/components/header/index.js b/packages/edit-site/src/components/header/index.js
index 2d03cf3e57..e930c45f4b 100644
--- a/packages/edit-site/src/components/header/index.js
+++ b/packages/edit-site/src/components/header/index.js
@@ -99,10 +99,10 @@ export default function Header( {
}
}, [ isInserterOpen, setIsInserterOpened ] );
- const toggleListView = useCallback(
- () => setIsListViewOpened( ! isListViewOpen ),
- [ setIsListViewOpened, isListViewOpen ]
- );
+ const { isListViewOpened } = useSelect( editSiteStore );
+ const toggleListView = useCallback( () => {
+ setIsListViewOpened( ! isListViewOpened() );
+ }, [] );
return (
<div className="edit-site-header">
I personally took the liberty of also removing the setter from the dependencies because I don't think that object will ever change, but that's up for debate. What really matters is that the callback itself no longer needs to be recomputed based on the change of a piece of state that the callback only cares about in an eventual future.
It's not as nice as setFoo( foo => ! foo )
, but it's a start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks @mcsf !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the testing steps and can confirm this has an impact via the profiler.
As shown, I can still do the "mash a ton of keys" and cause what feels like throttled typing. But it still feels like a rather big improvement over trunk.
Im actually not able to notice any impact without the profiler. I set up a post with ~100 buttons blocks and interactions with typing, opening list view / block inserter, all feel about the same.
Overall I think it makes sense to avoid anonymous functions for event handlers and this change is good.
Talking myself through why this has an impact, but I may be missing something: these components are updating and running through their code regardless of the change here (due to a parent update I assume). What changes here is that the comparison of the component output to whats on the DOM no longer sees a false difference due to the anonymous function reference, thus a patch to the DOM is no longer triggered every time the component updates, thats a great improvement! However, what im wondering about now is why do these header toolbar components update at all in this case. Why does typing in the editor need to trigger a parent to update for components in the header toolbar? Is this something we can restructure and avoid in general? 🤔
While I don't have anything against this particular PR, I don't think we should be doing this systematically, rerendering a component doesn't have a big impact on performance, sometimes the memoization actually costs more. Where it starts to be more problematic is for rerendering that trigger a chain (higher in the tree) or things that grow with content (for example So components in the chrome of the UI are not that impactful. I think ideally we'd find good measures to improve before doing such optimizations. |
This is one of the last of my low hanging fruit PRs of 🔪 what comes up near the top of the profiler. I agree that we don't want to use useCallback/useMemo everywhere, but this does surprisingly show up on the profiler where I'd expect the worst areas to be in the blocklist or child blocks. @youknowriad would you prefer we close this one out, or should we land this? It's true that this doesn't fix the general typing jank that folks can easily run into. I was going to look at a few experimental strategies next to test out if we can defer work in less important areas. For example can we throttle the chrome update/other secondary parts of the editor like the chrome, breadcrumb and list view? Are we really deferring work as expected when blocks are not in view? There's no need for the editor to look like this on every keypress: rerenders.mp4 |
The question is how much of these Christmas lights is actually a performance problem. I think the answer for me is not clear. I guess we could better measure it if we remove all those rerenderings at once maybe? For this PR, I honestly don't know how we can measure its impact properly, the PR itself doesn't hurt code base or anything so I'm fine with it being merged. your call really. |
Thanks for the reviews. I'll go ahead and merge this one. My next investigations 🔍 will likely focus on |
The ListView and Inserter Toolbar buttons show up next in the react profiler for many renders on a page with many navigation links, while typing in a new paragraph. In the header toolbar we can use
useCallback
to avoid passing a new function each time.This is also not a silver bullet for making things fast, but it does improve things and with this change in place NavigationLink will show up as a top slow area in the react profiler. (I'll look at that next).
See previous performance tweaks in: #32380, #32357, #32356
Before:
toolbarbefore.mp4
After:
headertoolbarafter.mp4
Testing Instructions