Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unmemoize Block component selectors #58355

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Unmemoize Block component selectors #58355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40f773f
Try: unmemoize block selectors
ellatrix Jan 28, 2024
e00b44b
Fix unit tests
ellatrix Jan 28, 2024
f3d63e6
Revert memo removal for selection ids
ellatrix Jan 29, 2024
73e3c78
Adjust mobile too
ellatrix Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,13 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { memo, useCallback, RawHTML, useContext } from '@wordpress/element';
import {
memo,
useCallback,
RawHTML,
useContext,
useMemo,
} from '@wordpress/element';
import {
getBlockType,
getSaveContent,
Expand Down Expand Up @@ -497,7 +503,8 @@ function BlockListBlockProvider( props ) {
getBlockMode,
isSelectionEnabled,
getTemplateLock,
__unstableGetBlockWithoutInnerBlocks,
getBlockWithoutAttributes,
getBlockAttributes,
canRemoveBlock,
canMoveBlock,

Expand All @@ -506,7 +513,7 @@ function BlockListBlockProvider( props ) {
getBlockEditingMode,
getBlockName,
isFirstMultiSelectedBlock,
getMultiSelectedBlockClientIds,
getSelectedBlockClientIdsUnmemoized,
hasSelectedInnerBlock,

getBlockIndex,
Expand All @@ -524,13 +531,14 @@ function BlockListBlockProvider( props ) {
__unstableGetEditorMode,
getSelectedBlocksInitialCaretPosition,
} = unlock( select( blockEditorStore ) );
const block = __unstableGetBlockWithoutInnerBlocks( clientId );
const blockWithoutAttributes =
getBlockWithoutAttributes( clientId );

// This is a temporary fix.
// 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.
if ( ! block ) {
if ( ! blockWithoutAttributes ) {
return;
}

Expand All @@ -542,7 +550,8 @@ function BlockListBlockProvider( props ) {
const templateLock = getTemplateLock( rootClientId );
const canRemove = canRemoveBlock( clientId, rootClientId );
const canMove = canMoveBlock( clientId, rootClientId );
const { name: blockName, attributes, isValid } = block;
const attributes = getBlockAttributes( clientId );
const { name: blockName, isValid } = blockWithoutAttributes;
const blockType = getBlockType( blockName );
const match = getActiveBlockVariation( blockName, attributes );
const { outlineMode, supportsLayout } = getSettings();
Expand All @@ -562,11 +571,7 @@ function BlockListBlockProvider( props ) {
isLocked: !! templateLock,
canRemove,
canMove,
// Users of the editor.BlockListBlock filter used to be able to
// access the block prop.
// Ideally these blocks would rely on the clientId prop only.
// This is kept for backward compatibility reasons.
block,
blockWithoutAttributes,
name: blockName,
attributes,
isValid,
Expand All @@ -578,7 +583,7 @@ function BlockListBlockProvider( props ) {
mayDisplayControls:
_isSelected ||
( isFirstMultiSelectedBlock( clientId ) &&
getMultiSelectedBlockClientIds().every(
getSelectedBlockClientIdsUnmemoized().every(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mayDisplayControls etc. should be selectors instead, but it's private anyway so it matters little.

( id ) => getBlockName( id ) === blockName
) ),
mayDisplayParentControls:
Expand Down Expand Up @@ -634,7 +639,7 @@ function BlockListBlockProvider( props ) {
isLocked,
canRemove,
canMove,
block,
blockWithoutAttributes,
name,
attributes,
isValid,
Expand Down Expand Up @@ -665,6 +670,15 @@ function BlockListBlockProvider( props ) {
defaultClassName,
} = selectedProps;

// Users of the editor.BlockListBlock filter used to be able to
// access the block prop.
// Ideally these blocks would rely on the clientId prop only.
// This is kept for backward compatibility reasons.
const block = useMemo(
() => ( { ...blockWithoutAttributes, attributes } ),
[ blockWithoutAttributes, attributes ]
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this is better than selector cache is because it's stored by component instance, not globally.


// Block is sometimes not mounted at the right time, causing it be
// undefined see issue for more info
// https://github.com/WordPress/gutenberg/issues/17013
Expand Down
36 changes: 17 additions & 19 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { INSERTER_PATTERN_TYPES } from '../components/inserter/block-patterns-ta
import { store } from './';
import { unlock } from '../lock-unlock';

export { getSelectedBlockClientIdsUnmemoized } from './utils';

/**
* Returns true if the block interface is hidden, or false otherwise.
*
Expand All @@ -44,6 +46,10 @@ export function getLastInsertedBlocksClientIds( state ) {
return state?.lastBlockInserted?.clientIds;
}

export function getBlockWithoutAttributes( state, clientId ) {
return state.blocks.byClientId.get( clientId );
}

/**
* Returns true if all of the descendants of a block with the given client ID
* have an editing mode of 'disabled', or false otherwise.
Expand All @@ -53,25 +59,17 @@ export function getLastInsertedBlocksClientIds( state ) {
*
* @return {boolean} Whether the block descendants are disabled.
*/
export const isBlockSubtreeDisabled = createSelector(
( state, clientId ) => {
const isChildSubtreeDisabled = ( childClientId ) => {
return (
getBlockEditingMode( state, childClientId ) === 'disabled' &&
getBlockOrder( state, childClientId ).every(
isChildSubtreeDisabled
)
);
};
return getBlockOrder( state, clientId ).every( isChildSubtreeDisabled );
},
( state ) => [
state.blocks.parents,
state.blocks.order,
state.blockEditingModes,
state.blockListSettings,
]
);
export const isBlockSubtreeDisabled = ( state, clientId ) => {
const isChildSubtreeDisabled = ( childClientId ) => {
return (
getBlockEditingMode( state, childClientId ) === 'disabled' &&
getBlockOrder( state, childClientId ).every(
isChildSubtreeDisabled
)
);
};
return getBlockOrder( state, clientId ).every( isChildSubtreeDisabled );
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #58349, this is no longer called, so the performance improvement of this PR will be lessened.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably still worth to remove the memoization I think... Also the dependencies of this selector weren't fully correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().


/**
* Returns a tree of block objects with only clientID and innerBlocks set.
Expand Down
58 changes: 19 additions & 39 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
checkAllowListRecursive,
checkAllowList,
getAllPatternsDependants,
getSelectedBlockClientIdsUnmemoized,
} from './utils';
import { orderBy } from '../utils/sorting';
import { STORE_NAME } from './constants';
Expand Down Expand Up @@ -757,39 +758,7 @@ export function getSelectedBlocksInitialCaretPosition( state ) {
* @return {Array} Multi-selected block client IDs.
*/
export const getSelectedBlockClientIds = createSelector(
( state ) => {
const { selectionStart, selectionEnd } = state.selection;

if ( ! selectionStart.clientId || ! selectionEnd.clientId ) {
return EMPTY_ARRAY;
}

if ( selectionStart.clientId === selectionEnd.clientId ) {
return [ selectionStart.clientId ];
}

// Retrieve root client ID to aid in retrieving relevant nested block
// order, being careful to allow the falsey empty string top-level root
// by explicitly testing against null.
const rootClientId = getBlockRootClientId(
state,
selectionStart.clientId
);

if ( rootClientId === null ) {
return EMPTY_ARRAY;
}

const blockOrder = getBlockOrder( state, rootClientId );
const startIndex = blockOrder.indexOf( selectionStart.clientId );
const endIndex = blockOrder.indexOf( selectionEnd.clientId );

if ( startIndex > endIndex ) {
return blockOrder.slice( endIndex, startIndex + 1 );
}

return blockOrder.slice( startIndex, endIndex + 1 );
},
getSelectedBlockClientIdsUnmemoized,
( state ) => [
state.blocks.order,
state.selection.selectionStart.clientId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand why unmemoizing getSelectedBlockClientIds in particular should be faster. You write:

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

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:

  1. At the top there is a WeakMap keyed by state.blocks.order. It should have just one record most of the time, because the old order array value are forgotten when the reducer state changes.
  2. The value of the WeakMap<Order> record is another WeakMap, keyed by a special LEAF_KEY object, indicating there are no more object-like dependant values.
  3. 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.
  4. This one [] record contains the memoized return value.
  5. The [] record is cleared when the current clientId is different from the last clientId. In other words, rememo remembers only the result for the last clientId used.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the cache would contain a long linked list of memoized result values for each clientId argument value.

Yes, that's true

Expand Down Expand Up @@ -852,7 +821,10 @@ export const getMultiSelectedBlocks = createSelector(
* @return {?string} First block client ID in the multi-selection set.
*/
export function getFirstMultiSelectedBlockClientId( state ) {
return getMultiSelectedBlockClientIds( state )[ 0 ] || null;
if ( ! hasMultiSelection( state ) ) {
return null;
}
return getSelectedBlockClientIdsUnmemoized( state )[ 0 ];
}

/**
Expand All @@ -864,8 +836,11 @@ export function getFirstMultiSelectedBlockClientId( state ) {
* @return {?string} Last block client ID in the multi-selection set.
*/
export function getLastMultiSelectedBlockClientId( state ) {
const selectedClientIds = getMultiSelectedBlockClientIds( state );
return selectedClientIds[ selectedClientIds.length - 1 ] || null;
if ( ! hasMultiSelection( state ) ) {
return null;
}
const selectedClientIds = getSelectedBlockClientIdsUnmemoized( state );
return selectedClientIds[ selectedClientIds.length - 1 ];
}

/**
Expand All @@ -892,7 +867,12 @@ export function isFirstMultiSelectedBlock( state, clientId ) {
* @return {boolean} Whether block is in multi-selection set.
*/
export function isBlockMultiSelected( state, clientId ) {
return getMultiSelectedBlockClientIds( state ).indexOf( clientId ) !== -1;
if ( isBlockSelected( state, clientId ) ) {
return false;
}
return (
getSelectedBlockClientIdsUnmemoized( state ).indexOf( clientId ) !== -1
);
}

/**
Expand Down Expand Up @@ -1004,7 +984,7 @@ export function __unstableIsSelectionCollapsed( state ) {
}

export function __unstableSelectionHasUnmergeableBlock( state ) {
return getSelectedBlockClientIds( state ).some( ( clientId ) => {
return getSelectedBlockClientIdsUnmemoized( state ).some( ( clientId ) => {
const blockName = getBlockName( state, clientId );
const blockType = getBlockType( blockName );
return ! blockType.merge;
Expand Down Expand Up @@ -2938,7 +2918,7 @@ export const isGroupable = createRegistrySelector(
const groupingBlockName = getGroupingBlockName();
const _clientIds = clientIds?.length
? clientIds
: getSelectedBlockClientIds( state );
: getSelectedBlockClientIdsUnmemoized( state );
const rootClientId = _clientIds?.length
? getBlockRootClientId( state, _clientIds[ 0 ] )
: undefined;
Expand Down
38 changes: 38 additions & 0 deletions packages/block-editor/src/store/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* Internal dependencies
*/
import { getBlockRootClientId, getBlockOrder } from './selectors';

export const checkAllowList = ( list, item, defaultResult = null ) => {
if ( typeof list === 'boolean' ) {
return list;
Expand Down Expand Up @@ -49,3 +54,36 @@ export const getAllPatternsDependants = ( state ) => {
state.blockPatterns,
];
};

const EMPTY_ARRAY = [];

export function getSelectedBlockClientIdsUnmemoized( state ) {
const { selectionStart, selectionEnd } = state.selection;

if ( ! selectionStart.clientId || ! selectionEnd.clientId ) {
return EMPTY_ARRAY;
}

if ( selectionStart.clientId === selectionEnd.clientId ) {
return [ selectionStart.clientId ];
}

// Retrieve root client ID to aid in retrieving relevant nested block
// order, being careful to allow the falsey empty string top-level root
// by explicitly testing against null.
const rootClientId = getBlockRootClientId( state, selectionStart.clientId );

if ( rootClientId === null ) {
return EMPTY_ARRAY;
}

const blockOrder = getBlockOrder( state, rootClientId );
const startIndex = blockOrder.indexOf( selectionStart.clientId );
const endIndex = blockOrder.indexOf( selectionEnd.clientId );

if ( startIndex > endIndex ) {
return blockOrder.slice( endIndex, startIndex + 1 );
}

return blockOrder.slice( startIndex, endIndex + 1 );
}
Loading