Block editor: optimise getGlobalBlockCount/getClientIdsWithDescendants #58356
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
|
Size Change: +24 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/optimise-get-global-block-count
branch
from
9964930 to
27eb256
Compare
ellatrix
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Flaky tests detected in 27eb256. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7686964824
|
ellatrix
changed the title
dmsnell
reviewed
Jan 29, 2024
| } | ||
| index++; |
There was a problem hiding this comment.
what's the role of index here? it doesn't seem to be tracking the end of the array, but it looks like it could intermix the ids returned in order from different descendants.
we don't want to push these to the end of the array, right? if we insert 5 descendant ids, do we want to add the next descendants inside of those, or do we want to increment index by 5 or 6 instead?
There was a problem hiding this comment.
We want to also search all the newly added clientIds for descendants, so we shouldn't increment. index just keeps increasing until we looped over all descendants and nested descendants.
There was a problem hiding this comment.
so it doesn't matter in which order we add the descendent ids to the list?
did you consider using ids.push( ...order )?
There was a problem hiding this comment.
It does matter :) I used push at first, but the tests were expecting the ids in a certain order (descendants right after parent ID vs after all the parent siblings
There was a problem hiding this comment.
okay I'm confused then, because it looks like we only get descendants immediately after their parents if there are only one descendant per parent
There was a problem hiding this comment.
It's deeply adding all descendants, we keep expanding the array as we find more, so the loop will also include those newly added descendants
ellatrix
force-pushed
the
try/optimise-get-global-block-count
branch
from
4d58bec to
7a98afa
Compare
noisysocks
reviewed
Jan 29, 2024
| }, | ||
| ( state ) => [ state.blocks.order ] | ||
| ); | ||
| export const getClientIdsWithDescendants = ( state ) => |
There was a problem hiding this comment.
Not strictly related to your changes, but I've always thought it's awkward when writing application code that we have both of these functions. They're named similarly and I can never remember which is which. Could we deprecate getClientIdsWithDescendants and make it so that getClientIdsOfDescendants defaults to rootIds = ''?
There was a problem hiding this comment.
Yes, it's really weird that we have both of these functions, I can deprecate it in a follow-up
noisysocks
approved these changes
Jan 29, 2024
| let index = 0; | ||
|
|
||
| // Add the descendants of the descendants, recursively. | ||
| while ( index < ids.length ) { |
There was a problem hiding this comment.
I think using a stack (while ( id = stack.pop() )) for this would be easier to grok. But I suppose you'd have to push items onto the stack in reverse order which is probably slow.
Carry on 👍
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?
Remove the recursive function calls and skip loops over empty arrays.
Too bad we cannot use the
byClientIdsapparently: #11787.Why?
Performs 20x faster on the large test post.
Also improves site editor load by ~3%.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast