Improve the getBlock and getBlocks performance #34241
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
|
Size Change: +302 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
@Copons Hi there! in this PR I'm doing a performance related refactoring and I'm seeing a failure in the |
I'm not quite sure. The test works for me as well locally, and even performing the same test actions manually results in the expected, correct, behaviour. |
|
@Copons Thanks for chiming in, I think I found a fix in the latest commit. |
234dda4 to
d668acf
Compare
d668acf to
1454d6d
Compare
|
Should I dare asking for a review or is this "too big to have a review"? :P |
|
I can confirm that performance results are slightly better:
For the typing metrics, all results have improved on average 5-10% which is rather impressive. I'm surprised seeing also improvements in the loading time metric. Anyway, it only means that we want to land all those changes as soon as possible. I would even consider backporting to the Gutenberg plugin 11.4 release 😄 There were no changes to e2e tests which is a good sign as well. I will have a closer look at the updated logic later today. |
gziolo
reviewed
Aug 26, 2021
| ] | ||
| ); | ||
| return state.blocks.tree[ clientId ]; | ||
| } | ||
|
|
||
| export const __unstableGetBlockWithoutInnerBlocks = createSelector( |
There was a problem hiding this comment.
I see that __unstableGetBlockWithoutInnerBlocks is used in one more place. Any chances we could get rid of it like in other places?
It's removed now from the core/editor store which is another positive aspect of this PR 😄
There was a problem hiding this comment.
We can easily replace its usage with getBlock the problem is that the current usage is exposed in a withFilters call and I didn't want to leak "inner blocks" to the filter.
oandregal
reviewed
Aug 26, 2021
| @@ -1225,6 +1219,8 @@ Returns an action object used in signalling that blocks have been received. | |||
| Unlike resetBlocks, these should be appended to the existing known set, not | |||
| replacing. | |||
|
|
|||
| Todo: This should be deprecated | |||
There was a problem hiding this comment.
Was catching up with this PR and wanted to bring attention to this comment.
There was a problem hiding this comment.
yeah, I'll need to deprecate this action properly, it's not used anywhere.
oandregal
reviewed
Aug 26, 2021
| @@ -225,14 +225,11 @@ export default compose( [ | |||
|
|
|||
| const isReadOnly = getSettings().readOnly; | |||
|
|
|||
| const block = __unstableGetBlockWithoutInnerBlocks( clientId ); | |||
| const { attributes, name } = block || {}; | |||
| const { attributes, name } = getBlock( clientId ); | |||
There was a problem hiding this comment.
Can getBlock( clientId ) be null? If so, wouldn't it throw a type error because it can't unwrap the properties?
There was a problem hiding this comment.
I guess it can in some rare occasions, I'll restore the fallback.
|
Now that the |
|
the tree indeed duplicates the data in the state, it's denormalization for performance reasons. The attributes and byClientId are used in other selectors that could be more performant than tree access. So I don't have a precise answer here, I think it's worth a try to remove them but it will make the "tree" reducer a bit more complex as right now it relies on these other states to compute the objects. If you feel like it, you can give it a try |
|
mmm,
|
13 tasks
|
@youknowriad This PR seems to have introduced the issue mentioned here - #35664 edit: I think I have a fix for it. (#35827 fixes) |
This was referenced Oct 21, 2021
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.
The getBlock and getBlocks selector in the block-editor state represent one of our bottlenecks in terms of typing performance. It's important that they are as fast as possible.
In trunk we rely on a
cacheKeystored in the reducer to indicate whether the block object need to be recomputed or not. I believe we can do a bit better and make everything simpler by denormalizing the block objects and computing them directly in the reducer. This means we can remove the memoization from these selectors and just return the cached values.