Adds a cache key to the blocks reducer in order to optimize the getBlock selector #16407
Conversation
|
I think this PR represents something like 20-30% performance improvement when typing (I'd appreciate confirmation) |
| * | ||
| * @return {Function} Enhanced reducer function. | ||
| */ | ||
| const withPostMetaUpdateCacheReset = ( reducer ) => ( state, action ) => { |
There was a problem hiding this comment.
I hope this becomes unnecessary when we factor out meta sources? 😄
| const newState = reducer( state, action ); | ||
| const previousMetaValues = get( state, [ 'settings', '__experimentalMetaSource', 'value' ] ); | ||
| const nextMetaValues = get( newState, [ 'settings', '__experimentalMetaSource', 'value' ] ); | ||
| // If post meta values change, reset the cache key for all blocks |
There was a problem hiding this comment.
Seems a bit heavy-handed, though at least for the meta source, I guess we can assume it probably doesn't hapen that much. I can imagine if this were scaled out to cover something like sourced post properties and a title block (#16282), busting cache of every block for every keypress in the title field might be excessive.
Considering how we proceed with an implementation of #16282, this may become a non-concern.
There was a problem hiding this comment.
yes, that's what we currently have with the memoization as well. I agree it's too drastic.
| const nextMetaValues = get( newState.settings.__experimentalMetaSource, [ 'value' ] ); | ||
| // If post meta values change, reset the cache key for all blocks | ||
| if ( previousMetaValues !== nextMetaValues ) { | ||
| newState.blocks = { |
There was a problem hiding this comment.
Something to look into separately: This condition is satisfied when saving a post, regardless of whether there are any updates to post meta. Possibly redundant with custom sources work, depending on whether there's an issue with how we're updating/resetting post during save and needlessly creating/assigning new meta references when no effective changes have happened.
| } | ||
| newState.cache = state.cache ? state.cache : {}; | ||
|
|
||
| const addParentBlocks = ( clientIds ) => { |
There was a problem hiding this comment.
To clarify, when we're refreshing the cache value for a block which has inner blocks, we don't need to refresh the cache for those descendents?
There was a problem hiding this comment.
no, we only need to refresh for its parents. The descendent object stays the same.
| @@ -192,8 +170,7 @@ export const getBlock = createSelector( | |||
| }; | |||
| }, | |||
| ( state, clientId ) => [ | |||
| ...getBlockAttributes.getDependants( state, clientId ), | |||
There was a problem hiding this comment.
Can we safely remove this? I tend to think if there's another selector being called and it has dependants, we should have this sort of reference. We might be able to make some exceptions where we've accounted for the difference and are optimizing an especially hot path, but might at least be worth to document this exception.
I ask also because there's a few other places in this file we still include getBlockAttributes.getDependants, and similarly would we consider to replace those with references to the cache value instead? Unlike the above "guideline", it's not very clear when and why we can reference the cache instead of other dependants values.
There was a problem hiding this comment.
Yeah, so the cache value is basically saying, the block object need to be refreshed. It includes both the attributes and the inner blocks.
So if we only depend on the attributes of the block, using the cache key might result in the selector being called again even if not needed.
I can add a comment to clarify here.
There was a problem hiding this comment.
While testing this branch I was getting an error:
reducer.js:208 Uncaught TypeError: Cannot read property ‘settings’ of undefined
caused on line:
const previousMetaValues = get( state.settings.__experimentalMetaSource, [ ‘value’ ] );
Without the last commit, everything seems to work well.
Edit: this was before the last commit added by @aduth, the new commit fixes the problem and the branch is working correctly.
youknowriad
force-pushed
the
add/block-cache-key
branch
from
8e47cc5 to
b002d1a
Compare
youknowriad
requested review from
ajitbohra,
chrisvanpatten and
mkaz
as code owners
…rnmobile/track-unsupported-blocks * 'master' of https://github.com/WordPress/gutenberg: Bump plugin version to 6.1.0-rc.1 Update HTML anchor explaination text (#16142) Move post permalink to beneath title on mobile. (#16277) Export cloneBlock method to the mobile (#16441) Fix inconsistent references to Settings Sidebar (#16138) Adds a cache key to the blocks reducer in order to optimize the getBlock selector (#16407) Track the parent block to optimize hierarchy selectors (#16392)
Extracted from #16368
Blocked by #16392
The getBlock selector is the bottleneck in terms of performance in the editor when the number of blocks increase. This PR adds a cache key to the reducer which let us drop the expensive
getBlockDependantsCacheBust