Unmemoize Block component selectors #58355
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: -9 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 73e3c78. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7698771238
|
ellatrix
commented
Jan 28, 2024
| const block = useMemo( | ||
| () => ( { ...blockWithoutAttributes, attributes } ), | ||
| [ blockWithoutAttributes, attributes ] | ||
| ); |
There was a problem hiding this comment.
The reason this is better than selector cache is because it's stored by component instance, not globally.
ellatrix
force-pushed
the
try/block-unmemoize-selectors
branch
from
2554b5f
to
c06dc6d
Compare
ellatrix
changed the title
ellatrix
force-pushed
the
try/block-unmemoize-selectors
branch
from
c06dc6d
to
e00b44b
Compare
ellatrix
commented
Jan 29, 2024
| @@ -578,7 +583,7 @@ function BlockListBlockProvider( props ) { | |||
| mayDisplayControls: | |||
| _isSelected || | |||
| ( isFirstMultiSelectedBlock( clientId ) && | |||
| getMultiSelectedBlockClientIds().every( | |||
| getSelectedBlockClientIdsUnmemoized().every( | |||
There was a problem hiding this comment.
Maybe mayDisplayControls etc. should be selectors instead, but it's private anyway so it matters little.
jsnajdr
reviewed
Jan 29, 2024
|
|
||
| return blockOrder.slice( startIndex, endIndex + 1 ); | ||
| }, | ||
| getSelectedBlockClientIdsUnmemoized, | ||
| ( state ) => [ | ||
| state.blocks.order, | ||
| state.selection.selectionStart.clientId, |
There was a problem hiding this comment.
I don't fully understand why unmemoizing getSelectedBlockClientIds in particular should be faster. You write:
createSelectorneeds to keep a cache of all previous selector calls by arguments and then constantly look up cache for new calls. So if you have a 1000 blocks, that’s a huge loop of checks
This would be true if the selector had a clientId argument. Then the cache would contain a long linked list of memoized result values for each clientId argument value. But for this selector, the cache is a rather small structure:
- At the top there is a
WeakMapkeyed bystate.blocks.order. It should have just one record most of the time, because the oldorderarray value are forgotten when the reducer state changes. - The value of the
WeakMap<Order>record is anotherWeakMap, keyed by a specialLEAF_KEYobject, indicating there are no more object-like dependant values. - The value of the
WeakMap<LeafKey>record is a linked list that always has just one record, for the[]argument array, because the selector has no arguments. - This one
[]record contains the memoized return value. - The
[]record is cleared when the currentclientIdis different from the lastclientId. In other words,rememoremembers only the result for the lastclientIdused.
To summarize, there is just one three-level cache.get( order ).get( LEAF_KEY ).find( r => r.clientId === clientId ) lookup in structures that have only one member. For reference, look at rememo source and how we have the isUniqueByDependants === false case for string/number clientId.
So, I think that unmemoizing this particular selector is controversial: it returns an array that deserves to have stable references, and I don't see where the speedup would come from.
For other selectors that return boolean values, I think unmemoization makes sense. Especially the ones that have arguments. It it plausible that recalculating the result from scratch is faster than maintaining large memoization maps.
One other such selector that comes to mind is canInsertBlockType. I've been changing it in #58262.
There was a problem hiding this comment.
Then the cache would contain a long linked list of memoized result values for each clientId argument value.
Yes, that's true
ellatrix
commented
Jan 29, 2024
| ); | ||
| }; | ||
| return getBlockOrder( state, clientId ).every( isChildSubtreeDisabled ); | ||
| }; |
There was a problem hiding this comment.
After #58349, this is no longer called, so the performance improvement of this PR will be lessened.
There was a problem hiding this comment.
Probably still worth to remove the memoization I think... Also the dependencies of this selector weren't fully correct.
There was a problem hiding this comment.
Also, this selector should also check the editing mode of the top-level clientId, not just its children.
If I look at the only call in BlockListBlockProvider, I see that it's done anyway, checking blockEditingMode === 'disabled' && isBlockSubtreeDisabled().
jsnajdr
approved these changes
Jan 29, 2024
ellatrix
commented
Jan 29, 2024
| @@ -432,7 +435,7 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId } ) => { | |||
| // This function should never be called when a block is not present in | |||
| // the state. It happens now because the order in withSelect rendering | |||
| // is not correct. | |||
| const { name, attributes, isValid } = block || {}; | |||
There was a problem hiding this comment.
For mobile, there's not really a need to keep passing attributes in the block object because mobile is not extensible by third party plugins.
There was a problem hiding this comment.
I don't understand this because I see that the wrapped BlockListBlock component uses the attributes prop several times. For example, when calling getAccessibilityLabel. And that uses, for example, attributes.level to say "this is a level 2 heading".
There was a problem hiding this comment.
attributes yes, but not block.attributes?
There was a problem hiding this comment.
I see, you're assigning the attributes variable elsewhere, with getBlockAttributes :) Nevermind.
|
I can't see any improvement in the numbers anymore. Maybe because of continuously calling |
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?
When there's 1000s of blocks, selector cache lookups may be expensive. This selector cache is relatively useless, unless the final result is non primitive derived state. I think doing it otherwise is harmful because createSelector needs to keep a cache of all previous selector calls by arguments and then constantly look up cache for new calls. So if you have a 1000 blocks, that’s a huge loop of checks (for what otherwise could have been a simpler state calculation).
Why?
Update: the performance impact is now lessened because of #58349.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast