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

Update the attributes reducer to use a map instead of a regular object #46146

Merged
merged 4 commits into from
Nov 30, 2022
Merged

Update the attributes reducer to use a map instead of a regular object #46146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a732c9
Update the attributes reducer to use a map instead of a regular object
youknowriad Nov 29, 2022
0bf6077
Small fixes
youknowriad Nov 30, 2022
63a9295
Reducer shouldn't update with the same attribute value
youknowriad Nov 30, 2022
1a3d795
Remove leftover
youknowriad Nov 30, 2022
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
192 changes: 94 additions & 98 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,23 +111,6 @@ function getFlattenedBlockAttributes( blocks ) {
return flattenBlocks( blocks, ( block ) => block.attributes );
}

/**
* Returns an object against which it is safe to perform mutating operations,
* given the original object and its current working copy.
*
* @param {Object} original Original object.
* @param {Object} working Working object.
*
* @return {Object} Mutation-safe object.
*/
function getMutateSafeObject( original, working ) {
if ( original === working ) {
return { ...original };
}

return working;
}

/**
* Returns true if the two object arguments have the same keys, or false
* otherwise.
Expand Down Expand Up @@ -177,7 +160,7 @@ function buildBlockTree( state, blocks ) {
for ( const block of flattenedBlocks ) {
result[ block.clientId ] = Object.assign( result[ block.clientId ], {
...state.byClientId[ block.clientId ],
attributes: state.attributes[ block.clientId ],
attributes: state.attributes.get( block.clientId ),
innerBlocks: block.innerBlocks.map(
( subBlock ) => result[ subBlock.clientId ]
),
Expand Down Expand Up @@ -281,7 +264,9 @@ const withBlockTree =
[ action.clientId ]: {
...newState.tree[ action.clientId ],
...newState.byClientId[ action.clientId ],
attributes: newState.attributes[ action.clientId ],
attributes: newState.attributes.get(
action.clientId
),
},
},
[ action.clientId ],
Expand All @@ -293,7 +278,7 @@ const withBlockTree =
( result, clientId ) => {
result[ clientId ] = {
...newState.tree[ clientId ],
attributes: newState.attributes[ clientId ],
attributes: newState.attributes.get( clientId ),
};
return result;
},
Expand Down Expand Up @@ -417,15 +402,15 @@ const withBlockTree =
break;
}
case 'SAVE_REUSABLE_BLOCK_SUCCESS': {
const updatedBlockUids = Object.entries( newState.attributes )
.filter( ( [ clientId, attributes ] ) => {
return (
newState.byClientId[ clientId ].name ===
'core/block' &&
attributes.ref === action.updatedId
);
} )
.map( ( [ clientId ] ) => clientId );
const updatedBlockUids = [];
newState.attributes.forEach( ( attributes, clientId ) => {
if (
newState.byClientId[ clientId ].name === 'core/block' &&
attributes.ref === action.updatedId
) {
updatedBlockUids.push( clientId );
}
} );
youknowriad marked this conversation as resolved.
Show resolved Hide resolved

newState.tree = updateParentInnerBlocksInTree(
newState,
Expand All @@ -434,7 +419,7 @@ const withBlockTree =
...updatedBlockUids.reduce( ( result, clientId ) => {
result[ clientId ] = {
...newState.byClientId[ clientId ],
attributes: newState.attributes[ clientId ],
attributes: newState.attributes.get( clientId ),
innerBlocks:
newState.tree[ clientId ].innerBlocks,
};
Expand Down Expand Up @@ -602,7 +587,9 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
const newState = {
...state,
byClientId: getFlattenedBlocksWithoutAttributes( action.blocks ),
attributes: getFlattenedBlockAttributes( action.blocks ),
attributes: new Map(
Object.entries( getFlattenedBlockAttributes( action.blocks ) )
),
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
order: mapBlockOrder( action.blocks ),
parents: mapBlockParents( action.blocks ),
controlledInnerBlocks: {},
Expand Down Expand Up @@ -724,21 +711,16 @@ const withSaveReusableBlock = ( reducer ) => ( state, action ) => {
}

state = { ...state };

state.attributes = mapValues(
state.attributes,
( attributes, clientId ) => {
const { name } = state.byClientId[ clientId ];
if ( name === 'core/block' && attributes.ref === id ) {
return {
...attributes,
ref: updatedId,
};
}

return attributes;
state.attributes = new Map( state.attributes );
state.attributes.forEach( ( attributes, clientId ) => {
const { name } = state.byClientId[ clientId ];
if ( name === 'core/block' && attributes.ref === id ) {
state.attributes.set( clientId, {
...attributes,
ref: updatedId,
} );
}
);
} );
}

return reducer( state, action );
Expand Down Expand Up @@ -830,84 +812,98 @@ export const blocks = pipe(
return state;
},

attributes( state = {}, action ) {
attributes( state = new Map(), action ) {
switch ( action.type ) {
case 'RECEIVE_BLOCKS':
case 'INSERT_BLOCKS':
return {
...state,
...getFlattenedBlockAttributes( action.blocks ),
};
case 'INSERT_BLOCKS': {
const newState = new Map( state );
Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad I'm inclined to add a comment documenting the choice of operations for a tangible performance hot-path optimization here. It'd be easy to lose track of it in a future refactor

Copy link
Member

Choose a reason for hiding this comment

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

There are reasons why we normally don't put Maps or other non-serializable values in the Redux store. It's no longer showing the state in the Redux devtools for me after this PR.

I tried "Immer" as it's also a popular library in Redux land, but it turned out be way less performant.

I wonder if there's any benchmark on this? Seems like Immer should also be performant in most cases. Were we testing it in the production build?

(Side note: adopting Immer should also help us when we want to integrate yjs more closely into the system for collaborative editing.)

I think we should at least bring back the ability to debug values in the Redux devtools. Maybe the serialize option would help?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any specific reason why Maps should be avoided, other than Redux DevTools not working with them (which to me sounds like a Redux DevTools problem)?

The link only goes on to mention:

"It also ensures that the UI will update as expected."

I'm not sure which UI this refers to, nor why it would stop updating as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if there's any benchmark on this?

It's a benchmark I did on my own using the same test included in this PR. A refactor using immer is actually very simple but it was three times worse in terms of performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call on the "serialize" thing, I'll be fixing that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up here #46282

Object.entries(
getFlattenedBlockAttributes( action.blocks )
).forEach( ( [ key, value ] ) => {
newState.set( key, value );
} );
return newState;
}

case 'UPDATE_BLOCK':
case 'UPDATE_BLOCK': {
// Ignore updates if block isn't known or there are no attribute changes.
if (
! state[ action.clientId ] ||
! state.get( action.clientId ) ||
! action.updates.attributes
) {
return state;
}

return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
...action.updates.attributes,
},
};
const newState = new Map( state );
newState.set( action.clientId, {
...state.get( action.clientId ),
...action.updates.attributes,
} );
return newState;
}

case 'UPDATE_BLOCK_ATTRIBUTES': {
// Avoid a state change if none of the block IDs are known.
if ( action.clientIds.every( ( id ) => ! state[ id ] ) ) {
if ( action.clientIds.every( ( id ) => ! state.get( id ) ) ) {
return state;
}

const next = action.clientIds.reduce(
( accumulator, id ) => ( {
...accumulator,
[ id ]: Object.entries(
action.uniqueByBlock
? action.attributes[ id ]
: action.attributes ?? {}
).reduce( ( result, [ key, value ] ) => {
// Consider as updates only changed values.
if ( value !== result[ key ] ) {
result = getMutateSafeObject(
state[ id ],
result
);
result[ key ] = value;
}

return result;
}, state[ id ] ),
} ),
{}
);

if (
action.clientIds.every(
( id ) => next[ id ] === state[ id ]
)
) {
return state;
let hasChange = false;
const newState = new Map( state );
for ( const clientId of action.clientIds ) {
const updatedAttributeEntries = Object.entries(
action.uniqueByBlock
? action.attributes[ clientId ]
: action.attributes ?? {}
);
if ( updatedAttributeEntries.length === 0 ) {
continue;
}
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
let hasUpdatedAttributes = false;
const existingAttributes = state.get( clientId );
const newAttributes = {};
updatedAttributeEntries.forEach( ( [ key, value ] ) => {
if ( existingAttributes[ key ] !== value ) {
hasUpdatedAttributes = true;
newAttributes[ key ] = value;
}
} );
hasChange = hasChange || hasUpdatedAttributes;
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
if ( hasUpdatedAttributes ) {
newState.set( clientId, {
...existingAttributes,
...newAttributes,
} );
}
}

return { ...state, ...next };
return hasChange ? newState : state;
}

case 'REPLACE_BLOCKS_AUGMENTED_WITH_CHILDREN':
case 'REPLACE_BLOCKS_AUGMENTED_WITH_CHILDREN': {
if ( ! action.blocks ) {
return state;
}

return {
...omit( state, action.replacedClientIds ),
...getFlattenedBlockAttributes( action.blocks ),
};
const newState = new Map( state );
action.replacedClientIds.forEach( ( clientId ) => {
newState.delete( clientId );
} );
Object.entries(
getFlattenedBlockAttributes( action.blocks )
).forEach( ( [ key, value ] ) => {
newState.set( key, value );
} );
return newState;
}

case 'REMOVE_BLOCKS_AUGMENTED_WITH_CHILDREN':
return omit( state, action.removedClientIds );
case 'REMOVE_BLOCKS_AUGMENTED_WITH_CHILDREN': {
const newState = new Map( state );
action.removedClientIds.forEach( ( clientId ) => {
newState.delete( clientId );
} );
return newState;
}
}

return state;
Expand Down
8 changes: 4 additions & 4 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 @@ -69,8 +69,8 @@ export function getBlockName( state, clientId ) {
const socialLinkName = 'core/social-link';

if ( Platform.OS !== 'web' && block?.name === socialLinkName ) {
const attributes = state.blocks.attributes[ clientId ];
const { service } = attributes;
const attributes = state.blocks.attributes.get( clientId );
const { service } = attributes ?? {};

return service ? `${ socialLinkName }-${ service }` : socialLinkName;
}
Expand Down Expand Up @@ -105,7 +105,7 @@ export function getBlockAttributes( state, clientId ) {
return null;
}

return state.blocks.attributes[ clientId ];
return state.blocks.attributes.get( clientId );
}

/**
Expand Down Expand Up @@ -152,7 +152,7 @@ export const __unstableGetBlockWithoutInnerBlocks = createSelector(
},
( state, clientId ) => [
state.blocks.byClientId[ clientId ],
state.blocks.attributes[ clientId ],
state.blocks.attributes.get( clientId ),
]
);

Expand Down
32 changes: 32 additions & 0 deletions packages/block-editor/src/store/test/performance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* Internal dependencies
*/
import reducer from '../reducer';

describe( 'performance', () => {
const state = reducer( undefined, { type: '@@init' } );
const blocks = [];
for ( let i = 0; i < 100000; i++ ) {
blocks.push( {
clientId: `block-${ i }`,
attributes: { content: `paragraph ${ i }` },
innerBlocks: [],
} );
}
const preparedState = reducer( state, {
type: 'RESET_BLOCKS',
blocks,
} );

it( 'should update blocks', () => {
const updatedState = reducer( preparedState, {
type: 'UPDATE_BLOCK_ATTRIBUTES',
clientIds: [ 'block-10' ],
attributes: {
content: 'updated paragraph 10',
},
} );

expect( updatedState ).toBeDefined();
} );
} );
Loading