Block Editor: Add client ID trees selectors #29902
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
Copons
added
[Type] Performance
Copons
requested review from
mattwiebe,
adamziel,
youknowriad,
talldan,
vindl,
creativecoder,
david-szabo97,
jeyip,
noahtallen and
Addison-Stavlo
|
Size Change: +51 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Copons
force-pushed
the
add/client-id-tree-selectors
branch
from
d7df7b7 to
4a58e20
Compare
gwwar
reviewed
Mar 17, 2021
There was a problem hiding this comment.
@Copons the overall idea makes sense to me to slim down the memo'd selector to just clientIds. If __unstableGetBlockTree is in use elsewhere it might be worth a similar cleanup in a follow up, as that cached work is highly likely to be thrown out with any block change.
Feel free to summon me back with a ping, when unit tests are ready.
Once y'all have a good baseline, it might be a good chance to add a perf E2E test for the site editor as a follow up. See example in post-editor. Really small changes can cause perf regressions.
typing.mp4
Manual testing with typing on this branch:
packages/block-editor/src/components/block-navigation/block-slot.js
Outdated
Show resolved
Hide resolved
|
|
||
| const _selectedBlockClientId = getSelectedBlockClientId(); | ||
| const _rootBlocks = __unstableGetClientIdsTree(); | ||
| const _rootBlock = _selectedBlockClientId |
There was a problem hiding this comment.
General question, why the _ prefix?
There was a problem hiding this comment.
It's because those names are already declared in the upper scope.
The _ is just simpler than figure out some naming switcheroo that might compromise the readability.
(Also, I've seen it used plenty of times in Gutenberg, sometimes to mark a constant as "private", but often in this exact same context of select hooks.)
|
If things are still pretty expensive to update in the persistent list view, another idea might be to skip re-renders while the user is typing. There's core/block-editor state available for that via |
|
We should wrap this component with "AsyncProvider" to avoid rerendering this synchronously, it should improve the performance instantaneously. The other thing we could do later is instead of having a unique selector to retrieve the tree at the root component, have a selector to retrieve "children" blocks on each level, this means adding/removing blocks will only update the parent of that block and not the whole tree. (need to verify that it's more performant) |
@gwwar The only other use is Following the stack, it seems that in that case the blocks attributes are eventually used. @Addison-Stavlo do you have more insights on the function? Like, could it survive with just
Noice, I didn't think of the perf tests, will try to add them too along with unit!
@youknowriad TIL! (or at the very least: TIR, Today I Remembered 😄 ) The performance gain is obvious, although this new slimmer selector should make things even quicker. |
Copons
force-pushed
the
add/client-id-tree-selectors
branch
from
4a58e20 to
d826cf9
Compare
youknowriad
reviewed
Mar 18, 2021
packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
|
@gwwar I've added the unit tests, but I'm having some troubles with the performance ones. I'd rather improve the Site Editor perf tests separately, to get them in relative parity with the Post Editor ones, before adding something related to the Persistent List View. 🤔 |
| ); | ||
| const { selectBlock } = useDispatch( blockEditorStore ); | ||
|
|
||
| function selectEditorBlock( clientId ) { |
There was a problem hiding this comment.
Any reason to use function vs arrow function?
There was a problem hiding this comment.
Not really, no. I don't think there's any practical difference in this case.
|
This is awesome. I'd be very interest to see how much this and the AsyncProvider improves the drag and drop performance in list view, that was always a little choppy with lots of blocks. |
|
This is a huge improvement! 🚀 Before ~70% of the frames are dropped Screen.Recording.2021-03-22.at.11.22.19.movAfter ~30% of the frames are dropped Screen.Recording.2021-03-22.at.11.23.04.mov |
|
Well @david-szabo97 @talldan if you like this so much, feel free to approve it! |
talldan
approved these changes
Mar 22, 2021
7 tasks
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.
Description
As mentioned in #29636 (review), keeping the Persistent List View open slows down the editor to a crawl, especially noticeable when typing.
The cause is that the
BlockNavigationuses the__unstableGetBlockTreeselector, which depends onstate.blocks.attributes, which can change quite often, especially while typing, causing a ton of wasted re-renders of the whole list.This is not noticeable with the non-persistent List View, given that you can't edit your content with the list open.
Turns out that
BlockNavigationdoesn't really need to know all about blocks, but it can easily live with just theirclientIds, as long as they are nested like the actual block tree.In this PR I've created a new
__unstableGetClientIdsTreewhich is fundamentally similar to the full-fledged__unstableGetBlockTree, but only usesclientIds.The list doesn't re-render anymore when a block changes.
Although, the list does re-render when a block is added, removed, or moved around. The result is that there's still a noticeable delay between the click and the action outcome.
Notes
To avoid having to change the internal logic of
BlockNavigation(and to keep this PR slim), the tree is pre-formatted with the expected properties:[ { clientId, innerBlocks: [ { ... }, ... ] }, { ... }, ... ]I'm happy with rewriting all the things, but first I'd like to be sure this is the right way to go.
Same thing about the unit tests: I'll add them if we are cool with this approach.
I've also avoided to delete the
__unstableGetBlockTreeselector, as it's used elsewhere.How has this been tested?
Also double-check all the uses of the List View for regressions:
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: