Header toolbar: useCallback to avoid unnecessary rerenders #32406
Conversation
|
Size Change: +66 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
gwwar
force-pushed
the
try/toolbar-events-perf
branch
from
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.
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.
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.
Copons
requested review from
vindl,
jeyip,
mattwiebe,
Addison-Stavlo and
creativecoder
and removed request for
vindl
|
Love it. You might not have silver bullets on hand, but these bullets are starting to at least feel like they are silver alloy based. With enough of them, you'll slay this werewolf 🤘 In this case, I'm seeing a pretty impressive jump! 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. Nice work! |
There was a problem hiding this comment.
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
useCallbackto 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