Batch container block settings action calls #43958
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
youknowriad
requested review from
oandregal,
jorgefilipecosta and
ntsekouras
youknowriad
commented
Sep 7, 2022
|
Size Change: +194 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Here's the result of the PR so far. The numbers improve a lot but we're not back entirely to the previous results (before list block).
It's important to note that now the test post contains 1400 blocks where previously it was 1000. The reason is because when the post is loaded the content is migrated and list blocks with inner blocks are created. That difference in the number of blocks mean there are more components to render causing an impact as well on the numbers. It also looks like there are some e2e test failures due to my optimizations, which I'll have to figure out before merging this. |
youknowriad
added
the
Backport to WP 6.7 Beta/RC
ellatrix
reviewed
Sep 9, 2022
| @@ -98,7 +101,29 @@ export default function useNestedSettingsUpdate( | |||
| } | |||
|
|
|||
| if ( ! isShallowEqual( blockListSettings, newSettings ) ) { | |||
There was a problem hiding this comment.
What setting causes isShallowEqual to be false?
There was a problem hiding this comment.
none, it's just the first time this gets called, it needs to be batched across the 500 blocks that call it in the same render commit.
There was a problem hiding this comment.
Oh, ok. Why is that not initialised/preloaded in the store though?
There was a problem hiding this comment.
Sorry, am bit slow today. Because the information is in a React component of course.
ellatrix
reviewed
Sep 9, 2022
| @@ -68,19 +68,26 @@ export default function useInnerBlockTemplateSync( | |||
| innerBlocks, | |||
| template | |||
| ); | |||
|
|
|||
| if ( ! isEqual( nextBlocks, innerBlocks ) ) { | |||
There was a problem hiding this comment.
How come isEqual returns false so much here? Blocks shouldn't change during load?
There was a problem hiding this comment.
It doesn't, the change here was a consequence of the change above to fix ctrl + click in the inserter.
youknowriad
force-pushed
the
perf/optimize-initial-rendering-nested-blocks
branch
from
5ea4352 to
4930cc5
Compare
|
Hey @geriux This PR improves the performance of container blocks drastically but it seems I have a failing mobile unit test. Would you have time to help me understand and fix this failure? Thank you. |
Hey there 👋 I'll check it out 👍 |
|
@youknowriad I have a fix for the tests that are failing, I also tested and the list block works correctly with these new changes 🎉 . Do you want me to push those to your branch or should I create a PR using this branch as base? Let me know 😃 Thanks! |
|
Go push here. Thanks @geriux |
|
Mobile unit tests are passing now 🚀 let us know if you have any other issues. Thanks! |
|
@geriux Looks like there's a new failure (group block) this time. 😬 |
It is now merged ✅ |
- Remove V1 List block tests - Updates the insertion test to add triggering of the onLayout event for the block list
youknowriad
force-pushed
the
perf/optimize-initial-rendering-nested-blocks
branch
from
bca027b to
1c4aa64
Compare
mtias
reviewed
Sep 19, 2022
packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Matias Ventura <[email protected]>
youknowriad
force-pushed
the
perf/optimize-initial-rendering-nested-blocks
branch
from
fb834f1 to
04814d9
Compare
ntsekouras
approved these changes
Sep 19, 2022
packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js
Outdated
Show resolved
Hide resolved
| return state; | ||
| } | ||
|
|
||
| // TODO: This is a source of bug, as each time there's a change in timing, |
There was a problem hiding this comment.
I think we need to add more info here about what the bug actually is, for future reference. Maybe the link to the PR? 🤔
There was a problem hiding this comment.
There's no identified bug, it's just a bad pattern (returning undefined from a reducer) that can cause bugs quickly and these bugs are very hard to track.
Co-authored-by: Nik Tsekouras <[email protected]>
youknowriad
deleted the
perf/optimize-initial-rendering-nested-blocks
branch
ockham
pushed a commit
that referenced
this pull request
Sep 19, 2022
Co-authored-by: Nik Tsekouras <[email protected]> Co-authored-by: Gerardo <[email protected]> Co-authored-by: Matias Ventura <[email protected]>
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: c21b626 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
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.
What?
It seems that the introduction of the list block with inner blocks highlighted an issue with the initial rendering of container blocks. (You can see that the loading metric increased by 200%).
The issue is that each time a container block is rendered a
updateBlockListSettingsaction is triggered calling a potential attempt to re-render the components on the page. So if you load a post with 50 container blocks, it will try to re-rendered the components on the page 50 times.How?
This PR attempts to solve that by batching the calls to
updateBlockListSettingsto ensure they only trigger a single re-render.Testing Instructions
The performance job should tell us whether this PR solves the issue or not.