Improve Inserter block hover performance#26348
Merged
david-szabo97 merged 18 commits intomasterfrom Jan 11, 2021
Merged
Conversation
|
Size Change: +112 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
david-szabo97
added
[Feature] Inserter
david-szabo97
requested review from
ajitbohra,
chrisvanpatten,
ellatrix and
youknowriad
as code owners
david-szabo97
changed the title
david-szabo97
requested review from
Addison-Stavlo,
Copons,
jeyip,
mattwiebe,
noahtallen,
sgomes,
simison and
vindl
Contributor
Addison-Stavlo
left a comment
Addison-Stavlo
left a comment
There was a problem hiding this comment.
In testing locally without throttling im not really noticing any difference in use. However, inspecting with Chrome performance tools I see some things like the mouseOut between each item going from ~28ms down to ~5ms, so it seems to be making a difference!
I notice we are memo-ing a lot of things here. All in all we definitely seem to be getting a better end result, but I can't help but wonder if any of these more basic items aren't worth the overhead of the memo. 🤔
Contributor
ItsJonQ
reviewed
Oct 28, 2020
| onHover={ onHover } | ||
| filterValue={ filterValue } | ||
| /> | ||
| const getCurrentTab = useCallback( |
There was a problem hiding this comment.
Ooo. I really like this implementation. Memoizing the .map() callback for iterating renders :D
ItsJonQ
approved these changes
Oct 28, 2020
ItsJonQ
left a comment
ItsJonQ
left a comment
There was a problem hiding this comment.
🚀 from me! Thanks @david-szabo97 for taking this on :).
I took it for a spin. Both the Block Inserter (top left) and the inline Quick inserter continue to work as normal.
I opened up React's dev tools to check for re-renders. They're definitely more controlled this time, as demonstrated in your GIF.
The memoization implementations look good to me!
We're only scratching the surface with this one. But it's a wonderful start ⭐
Contributor
|
thanks for your work here, the inserter was not really optimized like ever. I share what @Addison-Stavlo shared above. Ideally, we'd try to understand exactly the bottleneck. That said, is there a way to add a measure to the performance tests to measure the impacts on actual numbers? Avoid rerenders doesn't always mean better performance. |
Contributor
Your best bet is probably using Puppeteer to automate a sequence of steps, and measuring how long the click events take, similar to what you're doing for editor performance. |
6 tasks
david-szabo97
force-pushed
the
try/improve-inserter-hover-performance
branch
from
8893488 to
0bb44f8
Compare
Member
Author
|
Merged a PR which adds inserter perf measurements.
Hover seems to be 30-40% faster in this PR. |
youknowriad
reviewed
Dec 16, 2020
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Good timing improvements. I do believe it's better if we identify precisely what components/props need to be memoized instead of just memorizing everything. We may have particular components that are not well written and memorizing hides the issue.
| } | ||
|
|
||
| export default BlockTypesList; | ||
| export default memo( BlockTypesList ); |
Contributor
There was a problem hiding this comment.
Anything in particular that requires this memo call?
| } | ||
|
|
||
| export default InserterListItem; | ||
| export default memo( InserterListItem ); |
Contributor
There was a problem hiding this comment.
I can see that being useful as we don't want to "rerender all items" when just one of them changes.
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Show resolved
Hide resolved
packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
Outdated
Show resolved
Hide resolved
| { __( 'A tip for using the block editor' ) } | ||
| </VisuallyHidden> | ||
| <Tips /> | ||
| const blocksTab = useMemo( |
Contributor
There was a problem hiding this comment.
I believe memoizing elements or render props is useless
Member
Author
There was a problem hiding this comment.
These elements are used inside a render function. In this case, the render function will return a totally new element every time it's called - which results in all items being rerendered. If we are using memorization then it will reuse the same element every time.
Contributor
There was a problem hiding this comment.
React already does diffing when it comes to element, so I'm not sure that's necessary. Also since this is more top level, it's less necessary than memorizing leafs. Do you think we can try removing the memoization and see what impact it has on the numbers?
Member
Author
There was a problem hiding this comment.
Before:
│ inserterHover │ '38.89 ms' │ '69.18 ms' │
│ minInserterHover │ '35.32 ms' │ '64.11 ms' │
│ maxInserterHover │ '48.13 ms' │ '76.42 ms' │
Contributor
There was a problem hiding this comment.
Interesting, so all the performance gain is coming from the memoization of the tabs?
Member
Author
There was a problem hiding this comment.
If you boot up React profiler and enable "highlight updates" then you can clearly see all the items are rerendering when the memo is removed.
Since the passed children is a function, it behaves differently than just a simple element. That's why it's creating a whole new tree without on every render without the memo.
Contributor
There was a problem hiding this comment.
Ok, I think I still need to understand why the top component is rerendering at all but let's bring back the memorization for now. Thanks for your work and explorations. Highly appreciated and sorry for the back and forth.
Member
Author
There was a problem hiding this comment.
No worries! My thanks for going on with this!
Contributor
|
Would you mind rebasing the PR one more time, the drag and drop that just landed in master (not sure if included here yet) could impact the numbers. |
This reverts commit 2321b74.
youknowriad
approved these changes
Dec 17, 2020
…r-hover-performance
…r-hover-performance
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
Description
When moving our mouse over the blocks in the inserter it doesn't feel smooth. These changes aim to make it a smoother experience by avoiding unnecessary rerenders.
How has this been tested?
yarn devScreenshots
Profiling and GIFs were done in the Site Editor. Both the performance gains are both the same for Post and Site editor since they are using the same shared components where I made the changes.
Chrome performance profiler (no throttling)
React performance profiler (no throttling)
Highlighting rerendered components
Types of changes
Performance
Checklist: