Navigation: Improve selector performance #40700
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
Mamaduka
added
[Type] Performance
Mamaduka
requested review from
mcsf,
youknowriad,
ZebulanStanphill,
getdave and
talldan
|
Size Change: -81 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
Mamaduka
commented
Apr 28, 2022
| const selectedBlockDescendants = getClientIdsOfDescendants( [ | ||
| selectedBlockId, | ||
| ] ); | ||
| const selectedBlockDescendants = getBlockOrder( selectedBlockId ); |
There was a problem hiding this comment.
I'm not sure if this is the best replacement, but it worked in my tests.
There was a problem hiding this comment.
I guess the difference is that getClientIdsOfDescendants creates a deep list of clientIds (all client ids in the hierarchy below the block), and getBlockOrder is shallow?
It doesn't seem to matter, because the code that uses this variable only checks for one child.
It might be good to rename some of the variables 'child' as 'descendants' always implies a deep search to me.
The other thing I'm wondering about is why it uses selectedBlockClientId rather than the block's own clientId. It seems like this is used for the 'remove submenu' toolbar button that will only show if the current block is the selected one.
edit: actually, it does show when more than one submenu is selected, but seems unusable. Maybe it needs to be in a specific toolbar group.
There was a problem hiding this comment.
I guess the difference is that getClientIdsOfDescendants creates a deep list of clientIds (all client ids in the hierarchy below the block), and getBlockOrder is shallow?
Best to my knowledge :)
It might be good to rename some of the variables 'child' as 'descendants' always implies a deep search to me.
Good suggestion. Thanks.
Mamaduka
added
the
Backport to WP Beta/RC
|
Ok, so I did some thorough testing on this. I cannot speak to the code itself, but this is a HUGE improvement. See the two videos below. Before: Before.mp4After: After.mp4 |
Mamaduka
force-pushed
the
try/fix-navigation-block-performance
branch
from
1f31dfa
to
990985b
Compare
talldan
approved these changes
Apr 29, 2022
| const selectedBlockDescendants = getClientIdsOfDescendants( [ | ||
| selectedBlockId, | ||
| ] ); | ||
| const selectedBlockDescendants = getBlockOrder( selectedBlockId ); |
There was a problem hiding this comment.
I guess the difference is that getClientIdsOfDescendants creates a deep list of clientIds (all client ids in the hierarchy below the block), and getBlockOrder is shallow?
It doesn't seem to matter, because the code that uses this variable only checks for one child.
It might be good to rename some of the variables 'child' as 'descendants' always implies a deep search to me.
The other thing I'm wondering about is why it uses selectedBlockClientId rather than the block's own clientId. It seems like this is used for the 'remove submenu' toolbar button that will only show if the current block is the selected one.
edit: actually, it does show when more than one submenu is selected, but seems unusable. Maybe it needs to be in a specific toolbar group.
Mamaduka
commented
Apr 29, 2022
| @@ -459,14 +453,7 @@ export default function NavigationLinkEdit( { | |||
| clientId, | |||
| true | |||
| ), | |||
| isImmediateParentOfSelectedBlock: hasSelectedInnerBlock( | |||
There was a problem hiding this comment.
Cleanup: The component wasn't using this.
Mamaduka
commented
Apr 29, 2022
| false | ||
| ), | ||
| hasDescendants: !! descendants, | ||
| selectedBlockHasDescendants: !! getClientIdsOfDescendants( [ |
There was a problem hiding this comment.
Same for selectedBlockHasDescendants.
Mamaduka
force-pushed
the
try/fix-navigation-block-performance
branch
from
62967c5
to
8da47fe
Compare
Mamaduka
changed the title
Mamaduka
changed the title
getdave
approved these changes
Apr 29, 2022
adamziel
pushed a commit
that referenced
this pull request
Apr 29, 2022
* Update variable names and cleanup
|
I cherry picked this change into |
adamziel
removed
the
Backport to WP Beta/RC
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?
See #40357.
PR tries to optimize the Navigation block performance.
Why?
DevTools performance tab was pointing to
rememoandgetClientIdsWithDescendantsstarted. Probably recent changes to the selector affected the performance.I'm not familiar with
remomointernals, but I think it's worth debugging separately.How?
Replaced
getClientIdsWithDescendantswithgetBlockCountandgetBlockOrder. Props to @mcsf for suggestion - #39985 (comment).Testing Instructions
Screenshots or screencast
After I started changing selectors
Trunk