Memoize getEntityRecords to prevent infinite re-renders #26447
Conversation
kevin940726
added
[Type] Bug
kevin940726
requested review from
draganescu,
adamziel,
noisysocks,
talldan and
tellthemachines
|
Question is: where should we memoize it? Should we memoize it in the selector level, or component level? Or else where? 馃 |
|
Hmmm, good question! 馃槄 I think it's reasonable to expect that I played around in DevTools to check that this is already the case when there is data:
So, I think the fix should be to change this: gutenberg/packages/core-data/src/selectors.js Lines 240 to 242 in a4af7d0 To use a shared reference to if ( ! queriedState ) {
return EMPTY_ARRAY;
}You can see that we've done similar optimisations in |
|
@noisysocks That's actually my first solution! However, the problem is that I feel like it's pretty normal to return a new instance of the data in the selectors. Take The other solution would be to memoize the selector using Secondly,
Note that even though (3) and (1) have the same arguments, (3) still couldn't reuse the cache from (1) due to the cache size.
|
I don't think that's normal, we explicitly avoid this across our entire code base, in general when we "map", we always memoize the selector itself unless it's a selector that is used rarely. I also lean towards memoizing in the selectors (using the shared array here)
I disagree, I feel it's more consistent to say that all selectors returning objects and arrays should be memoized rather than thinking adhoc in components.
I'm not sure that's true, if it is, it's a bug, we explicitly designed our selectors to keep the cache for different arguments and not just limit it to 1. I see that |
If that's the case then it makes sense to me! I didn't know that there's such practice in our code base, and from a quick search I can still find several places which aren't memoizing selectors. Is this documented somewhere? I'm not sure if it counts as idiomatic code, but would help if it's documented explicitly.
I feel like using |
true, but EMPTY_ARRAY is way less costfull.
We do have that document to explain some of the performance decisions but this one is definitely not there, it would be good to add. https://developer.wordpress.org/block-editor/architecture/performance/ This Post also has some good context about the different performant-related work. https://riad.blog/2020/02/14/a-journey-towards-a-performant-web-editor/ |
|
I wonder if we can automatically catch when selectors are incorrectly returning new instances by doing something like this in our unit tests. import { getEntityRecords as _getEntityRecords } from '../selectors';
const checkSelectorStability = ( selector ) => ( ...args ) => {
const firstResult = selector( ...args );
const secondResult = selector( ...args );
expect( secondResult ).toBe( firstResult );
return secondResult;
};
const getEntityRecords = checkSelectorStability( _getEntityRecords ); |
kevin940726
force-pushed
the
update/fix-reusable-blocks-rerenders
branch
2 times, most recently
from
b2d74e9
to
be1a741
Compare
|
Size Change: +14 B (0%) Total Size: 1.21 MB
鈩癸笍 View Unchanged
|
packages/core-data/src/selectors.js
Outdated
| if ( ! queriedState ) { | ||
| return []; | ||
| } | ||
| return getQueriedItems( queriedState, query ); | ||
| } | ||
| }, | ||
| ( state, kind, name ) => [ getQueriedState( state, kind, name ) ] |
There was a problem hiding this comment.
Will we have the same result if we use a const array? If it's the case, this seems to be way harder to understand and also more costly in terms of performance than just using a const.
There was a problem hiding this comment.
Good point. I don't think that there's a difference in this specific case. However, I'd still prefer to use a more general solution so that future changes won't accidentally break the memoization.
Let's use a const array here, as it's more simpler and performant. I still think it's too complicated to choose the right memoization method with the current approach though 馃 .
There was a problem hiding this comment.
Yes, I agree that memoization is hard in general. Redux forces us into thinking more about these details in a way.
kevin940726
force-pushed
the
update/fix-reusable-blocks-rerenders
branch
from
95eda75
to
6b1d661
Compare
kevin940726
changed the title
talldan
added
the
Backport to WP Minor Release
|
I've added the Backport to Minor Release label so that if there's a 5.6.2 release this will be included. If 5.6.2 doesn't happen and 5.7 is release the label can be removed, and it'll already be in 5.7. |
* Backport #26475 for the 5.6 branch. * Backport #28604. * Revert "Backport #28604." This reverts commit 32784b6. * Backport #26583 to the 5.6 branch. * Commit lock file updates. * Committing lock file differences after updating `browserslist`. * Pin the version of NodeJS in the Compressed Size workflow. * Memoize getEntityRecords to prevent infinite re-renders (#26447) * Fix issue where gallery block requests all attachments when empty (#28621) * Return early from building attachments for galleries * Improve code clarity * Fix link control styles to prevent scrollbar (#27777) * Update package-lock * Update package-lock again Co-authored-by: Jonathan Desrosiers <desrosj@users.noreply.github.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
noisysocks
removed
the
Backport to WP Minor Release
Description
Memoize
reusableBlocksto prevent infinite re-renders in some cases.reusableBlocksis called fromgetEntityRecordshere:gutenberg/packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
Lines 45 to 48 in a4af7d0
When the data hasn't arrived yet,
getEntityRecordswill return an empty array as defined here:gutenberg/packages/core-data/src/selectors.js
Lines 240 to 242 in a4af7d0
Keep in mind that, for every change to the store,
getEntityRecordswill return a new instance of the empty array.reusableBlocksis then passed touseMemobelow to form asettingsobject.gutenberg/packages/edit-widgets/src/components/widget-areas-block-editor-provider/index.js
Line 74 in a4af7d0
settingsis passed to<BlockEditorProvider>below, which schedules an effect to update the settings whenever it changes:gutenberg/packages/block-editor/src/components/provider/index.js
Lines 19 to 21 in a4af7d0
This process creates an infinite loop before the
quriedDataarrived.getEntityRecordsreturns a new empty array, hence a newreusableBlocks.reusableBlockschanges,useMemocomputes a newsettings.settingschanges,useEffectcallsupdateSettingsto update the store.useSelectcallsgetEntityRecordsagain and returns a new empty array, back to (1).This loop only ends when
getEntityRecordsreturns thequeriedDatareturned from the network request in (1).This PR tries to fix this by memoize
reusableBlocksso that we don't have to create a newsettingswhenevergetEntityRecordsreturns a new array, breaking the loop in (2).How has this been tested?
console.login theuseMemoofsettingsSee the screenshots below.
Screenshots
(It really shouldn't be
renderedin the log but hopefully you get the point 馃槄 )Types of changes
Bug fix
Checklist: