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

Improve typing performance by splitting attributes in state tree. #12312

Merged
merged 2 commits into from
Dec 20, 2018
+594 −195
Merged

Improve typing performance by splitting attributes in state tree. #12312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99f8a2b
Improve typing performance by splitting attributes in state tree.
sgomes Nov 26, 2018
cdd96ca
Revert changes to getBlocks dependants
sgomes Dec 4, 2018
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
213 changes: 154 additions & 59 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
omitBy,
keys,
isEqual,
isEmpty,
overSome,
get,
} from 'lodash';
Expand Down Expand Up @@ -78,29 +79,52 @@ function mapBlockOrder( blocks, rootClientId = '' ) {
}

/**
* Given an array of blocks, returns an object containing all blocks, recursing
* into inner blocks. Keys correspond to the block client ID, the value of
* which is the block object.
* Helper method to iterate through all blocks, recursing into inner blocks,
* applying a transformation function to each one.
* Returns a flattened object with the transformed blocks.
*
* @param {Array} blocks Blocks to flatten.
* @param {Function} transform Transforming function to be applied to each block.
*
* @return {Object} Flattened blocks object.
* @return {Object} Flattened object.
*/
function getFlattenedBlocks( blocks ) {
const flattenedBlocks = {};
function flattenBlocks( blocks, transform ) {
const result = {};

const stack = [ ...blocks ];
while ( stack.length ) {
// `innerBlocks` is redundant data which can fall out of sync, since
// this is reflected in `blocks.order`, so exclude from appended block.
const { innerBlocks, ...block } = stack.shift();

stack.push( ...innerBlocks );

flattenedBlocks[ block.clientId ] = block;
result[ block.clientId ] = transform( block );
}

return flattenedBlocks;
return result;
}

/**
* Given an array of blocks, returns an object containing all blocks, without
* attributes, recursing into inner blocks. Keys correspond to the block client
* ID, the value of which is the attributes object.
*
* @param {Array} blocks Blocks to flatten.
*
* @return {Object} Flattened block attributes object.
*/
function getFlattenedBlocksWithoutAttributes( blocks ) {
return flattenBlocks( blocks, ( block ) => omit( block, 'attributes' ) );
}

/**
* Given an array of blocks, returns an object containing all block attributes,
* recursing into inner blocks. Keys correspond to the block client ID, the
* value of which is the attributes object.
*
* @param {Array} blocks Blocks to flatten.
*
* @return {Object} Flattened block attributes object.
*/
function getFlattenedBlockAttributes( blocks ) {
return flattenBlocks( blocks, ( block ) => block.attributes );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

A nice little refactoring was suggested here by @noisysocks #12268 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Done.

/**
Expand Down Expand Up @@ -252,7 +276,11 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
...state,
byClientId: {
...omit( state.byClientId, visibleClientIds ),
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
},
attributes: {
...omit( state.attributes, visibleClientIds ),
...getFlattenedBlockAttributes( action.blocks ),
},
order: {
...omit( state.order, visibleClientIds ),
Expand All @@ -264,6 +292,43 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
return reducer( state, action );
};

/**
* Higher-order reducer which targets the combined blocks reducer and handles
Copy link
Member

Choose a reason for hiding this comment

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

What does "handles" entail? It's not immediately obvious to me as a reviewer now, let alone a future hypothetical maintainer.

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify a bit: This HoR only updates the attributes of blocks not the "byClientId" part but we need to access "byClientId" to know the name of the blocks as we only want to change the core/block blocks.

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'll add a clarification that this HoR exists because it needs access to both branches of the state tree simultaneously.

* the `SAVE_REUSABLE_BLOCK_SUCCESS` action. This action can't be handled by
* regular reducers and needs a higher-order reducer since it needs access to
* both `byClientId` and `attributes` simultaneously.
*
* @param {Function} reducer Original reducer function.
*
* @return {Function} Enhanced reducer function.
*/
const withSaveReusableBlock = ( reducer ) => ( state, action ) => {
if ( state && action.type === 'SAVE_REUSABLE_BLOCK_SUCCESS' ) {
const { id, updatedId } = action;

// If a temporary reusable block is saved, we swap the temporary id with the final one
if ( id === updatedId ) {
return state;
}

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;
} );
}

return reducer( state, action );
};

/**
* Undoable reducer returning the editor post state, including blocks parsed
* from current HTML markup.
Expand Down Expand Up @@ -342,6 +407,8 @@ export const editor = flow( [

withBlockReset,

withSaveReusableBlock,

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
withChangeDetection( {
Expand All @@ -352,63 +419,114 @@ export const editor = flow( [
byClientId( state = {}, action ) {
switch ( action.type ) {
case 'SETUP_EDITOR_STATE':
return getFlattenedBlocks( action.blocks );
return getFlattenedBlocksWithoutAttributes( action.blocks );

case 'RECEIVE_BLOCKS':
return {
...state,
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'UPDATE_BLOCK_ATTRIBUTES':
case 'UPDATE_BLOCK':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
return state;
}

// Consider as updates only changed values
const nextAttributes = reduce( action.attributes, ( result, value, key ) => {
if ( value !== result[ key ] ) {
result = getMutateSafeObject( state[ action.clientId ].attributes, result );
result[ key ] = value;
}

return result;
}, state[ action.clientId ].attributes );

// Skip update if nothing has been changed. The reference will
// match the original block if `reduce` had no changed values.
if ( nextAttributes === state[ action.clientId ].attributes ) {
// Do nothing if only attributes change.
Copy link
Member

Choose a reason for hiding this comment

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

What does / should this do if the update is for innerBlocks?

Copy link
Contributor Author

@sgomes sgomes Nov 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is it valid semantics to modify inner blocks with an UPDATE_BLOCK instead of MOVE_* and other actions? I assumed not, since that possibility isn't covered in the existing code. Or rather, I assumed no special attention would need to be paid to it. Should I add a check to discard any innerBlocks changes?

Copy link
Member

Choose a reason for hiding this comment

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

Is it valid semantics to modify inner blocks with an UPDATE_BLOCK instead of MOVE_* and other actions?

It's a downside of the "update" action being so generic, but hypothetically it should be able to expect receiving any properties of a block object (the value returned by createBlock).

Copy link
Member

Choose a reason for hiding this comment

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

While I think it's something we should respect, we already don't have consideration for a changed innerBlocks from the order subtree, so I think it'd be fine to defer.

const changes = omit( action.updates, 'attributes' );
if ( isEmpty( changes ) ) {
return state;
}

// Otherwise merge attributes into state
return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
attributes: nextAttributes,
...changes,
},
};

case 'INSERT_BLOCKS':
return {
...state,
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'REPLACE_BLOCKS':
if ( ! action.blocks ) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this was inherited, but looking at it, it seems quite wrong. We call replaceBlocks with an empty array when, for example, removing a paragraph:

gutenberg/packages/block-library/src/paragraph/edit.js

Lines 116 to 117 in b5d4e15

// If before content is omitted, treat as intent to delete block.
onReplace( [] );

If I'm reading this right, this behavior would leave lingering state for the removed block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An empty array is truthy, so it seems to me that this should work correctly, unless I'm missing something?

Copy link
Member

Choose a reason for hiding this comment

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

An empty array is truthy, so it seems to me that this should work correctly, unless I'm missing something?

That's a good point. Makes me wonder why the condition exists at all then 🤷‍♂️

return state;
}

return {
...omit( state, action.clientIds ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'REMOVE_BLOCKS':
return omit( state, action.clientIds );
}

return state;
},

attributes( state = {}, action ) {
switch ( action.type ) {
case 'SETUP_EDITOR_STATE':
return getFlattenedBlockAttributes( action.blocks );

case 'RECEIVE_BLOCKS':
return {
...state,
...getFlattenedBlockAttributes( action.blocks ),
};

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

return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
...action.updates,
...action.updates.attributes,
},
};

case 'UPDATE_BLOCK_ATTRIBUTES':
Copy link
Member

Choose a reason for hiding this comment

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

Kinda makes me wish we'd normalized UPDATE_BLOCK_ATTRIBUTES to use UPDATE_BLOCK, but it's also not something I think we ought to be addressing here.

// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
return state;
}

// Consider as updates only changed values
const nextAttributes = reduce( action.attributes, ( result, value, key ) => {
if ( value !== result[ key ] ) {
result = getMutateSafeObject( state[ action.clientId ], result );
result[ key ] = value;
}

return result;
}, state[ action.clientId ] );

// Skip update if nothing has been changed. The reference will
// match the original block if `reduce` had no changed values.
if ( nextAttributes === state[ action.clientId ] ) {
return state;
}

// Otherwise replace attributes in state
return {
...state,
[ action.clientId ]: nextAttributes,
};

case 'INSERT_BLOCKS':
return {
...state,
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlockAttributes( action.blocks ),
};

case 'REPLACE_BLOCKS':
Expand All @@ -418,34 +536,11 @@ export const editor = flow( [

return {
...omit( state, action.clientIds ),
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlockAttributes( action.blocks ),
};

case 'REMOVE_BLOCKS':
return omit( state, action.clientIds );

case 'SAVE_REUSABLE_BLOCK_SUCCESS': {
const { id, updatedId } = action;

// If a temporary reusable block is saved, we swap the temporary id with the final one
if ( id === updatedId ) {
return state;
}

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

return block;
} );
}
}

return state;
Expand Down
23 changes: 10 additions & 13 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ export const getBlockAttributes = createSelector(
return null;
}

let { attributes } = block;
let attributes = state.editor.present.blocks.attributes[ clientId ];

// Inject custom source attribute values.
//
Expand All @@ -667,6 +667,7 @@ export const getBlockAttributes = createSelector(
},
( state, clientId ) => [
state.editor.present.blocks.byClientId[ clientId ],
state.editor.present.blocks.attributes[ clientId ],
state.editor.present.edits.meta,
state.initialEdits.meta,
state.currentPost.meta,
Expand Down Expand Up @@ -698,9 +699,8 @@ export const getBlock = createSelector(
};
},
( state, clientId ) => [
state.editor.present.blocks.byClientId[ clientId ],
getBlockDependantsCacheBust( state, clientId ),
...getBlockAttributes.getDependants( state, clientId ),
getBlockDependantsCacheBust( state, clientId ),
]
);

Expand Down Expand Up @@ -729,9 +729,7 @@ export const getBlocks = createSelector(
( clientId ) => getBlock( state, clientId )
);
},
( state ) => [
state.editor.present.blocks,
]
( state ) => [ state.editor.present.blocks ]
);

/**
Expand Down Expand Up @@ -1125,10 +1123,8 @@ export const getMultiSelectedBlocks = createSelector(
return multiSelectedBlockClientIds.map( ( clientId ) => getBlock( state, clientId ) );
},
( state ) => [
state.editor.present.blocks.order,
state.blockSelection.start,
state.blockSelection.end,
state.editor.present.blocks.byClientId,
...getMultiSelectedBlockClientIds.getDependants( state ),
state.editor.present.blocks,
state.editor.present.edits.meta,
state.initialEdits.meta,
state.currentPost.meta,
Expand Down Expand Up @@ -1178,7 +1174,7 @@ const isAncestorOf = createSelector(
return possibleAncestorId === idToCheck;
},
( state ) => [
state.editor.present.blocks,
state.editor.present.blocks.order,
Copy link
Member

Choose a reason for hiding this comment

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

Are these revisions relevant to the proposed change? Or are these things which are technically separate issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, these changes are one of the important changes in this PR as we don't invalidate these selectors unless the inserted blocks (order) change instead of any attribute change.

Copy link
Member

Choose a reason for hiding this comment

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

I can see it would have an important impact on the overall benefit of the memoization; but is it specifically related to pulling out attributes to a separate state path?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you're right, this one in particular is not directly related.

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'm happy enough to create a separate PR for this, but it doesn't seem like it would be outlandish to keep it in this PR either.

],
);

Expand Down Expand Up @@ -1985,7 +1981,8 @@ export const getInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.byClientId,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably need .order here as well right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I'm not certain

Copy link
Contributor Author

@sgomes sgomes Nov 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

From what I can tell, block order should not affect inserter items, but I could be wrong. Is there someone with more domain-specific knowledge of inserter items that could weigh in?

Copy link
Member

Choose a reason for hiding this comment

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

Nested blocks might have some impact on that, but as noted in the other PR, I would love us to explore whether we can skip it here.

Copy link
Member

Choose a reason for hiding this comment

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

getInserterItems() relies on .order in two ways:

  1. It uses getClientIdsWithDescendants() which in turn uses .order. This is to set an inserter item's disabled attribute which is true when a block has already been used.
  2. It uses canIncludeReusableBlockInInserter() which in turn uses isAncestorOf() which in turn uses .order This is to stop reusable column blocks from being inserted into themselves which causes an infinite loop.

(1) should be safe to ignore because inserting or removing a block modifies .byClientId as well. (2) I'm less sure about—I think it's safe to ignore because one cannot create that infinite loop scenario by only re-ordering blocks.

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'm not too concerned with the performance implications of depending on .order, from what I've seen, so I'll add in that dependency just to be on the safe side. If this were to produce any issues due to the intricate dependencies you're describing, it could get extremely difficult to debug, so it's not worth the risk.

Copy link
Member

Choose a reason for hiding this comment

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

so I'll add in that dependency just to be on the safe side.

It still needs to be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had forgotten to push my changes.

state.editor.present.blocks.order,
state.preferences.insertUsage,
state.settings.allowedBlockTypes,
state.settings.templateLock,
Expand Down Expand Up @@ -2019,7 +2016,7 @@ export const hasInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.byClientId,
state.settings.allowedBlockTypes,
state.settings.templateLock,
state.reusableBlocks.data,
Expand Down
Loading