Extract the attributes from the editor's state shape to enhance performance #12268
Conversation
|
Thanks, @youknowriad ! I've got a similar set of changes I'm working on, and are nearly done save for the tests, which still need some fixes. I also modified some of the other selectors to not rely on attributes, wherever possible, and added some memoization here and there. For improvements, I measured 30% better results before I started fixing the tests; I'll run some benchmarks again once I'm finished there. Shall I post a WIP of this on Monday so that we can compare PRs and decide which one to go with? |
|
Sure, just push your PR when you're ready. My PR is very basic, just applying the root idea to check the impact without any other improvements. Happy to continue based on your work. |
There was a problem hiding this comment.
The approach makes a lot of sense to me, and I am seeing a 1.6x (18 ms to 11 ms) reduction in time spent evaluating JavaScript while processing keypress events which is super encouraging.
I'm reluctant to include this in 5.0 since this has the potential to introduce unexpected regressions very late into the release process.
I'm seeing a few bugs when I test locally.
Pressing Backspace causes blocks to unexpectedly merge
- Create a new post
- Type some text
- Press Enter to create a new paragraph
- Type some more text
- Press Backspace
Expected result: The last character should be deleted.
Actual result: The two paragraph blocks merge.
Autosaves fail
- Create a new post
- Type some text
- Without deselecting the block, wait for an autosave to happen
Expected result: The post should autosave.
Actual result: The autosave fails and an Updating failed message appears.
Changing an existing block does not mark the post as dirty
- Create a new post
- Enter a title
- Type some text into the initial paragraph block
- Deselect the block
- Click Save Draft
- Re-select the block and modify its text
Expected result: The Save Draft button should become active.
Actual result: The Save Draft button remains disabled.
The / inserter no longer works
- Create a new post
- Select the initial paragraph block
- Type /
Expected result: The block autocompleted should appear.
Actual result: A / is typed and nothing else.
| } | ||
|
|
||
| return flattenedBlockAttributes; | ||
| } |
There was a problem hiding this comment.
getFlattenedBlocks and getFlattenedBlockAttributes are very similar. They're both functions that flatten a block tree into a hash map. The only difference is what is stored in the map. Could we use a higher order function that leaves it up to the caller to decide what is stored in the map?
function flattenBlocks( blocks, iteratee ) {
const result = {};
const stack = [ ...blocks ];
while ( stack.length ) {
const { innerBlocks, ...block } = stack.shift();
stack.push( ...innerBlocks );
result[ block.clientId ] = iteratee( block );
}
return result;
}| ...state, | ||
| [ action.clientId ]: { | ||
| ...state[ action.clientId ], | ||
| ...omit( action.updates, 'attributes' ), |
There was a problem hiding this comment.
We don't need to call omit() again here.
| ...omit( action.updates, 'attributes' ), | |
| ...changes, |
| } | ||
|
|
||
| const changes = omit( action.updates, 'attributes' ); | ||
| if ( keys( changes ).length === 0 ) { |
There was a problem hiding this comment.
nit: We could use _.isEmpty() here, though not sure how it affects performance.
if ( isEmpty( changes ) ) {| @@ -1947,7 +1948,8 @@ export const getInserterItems = createSelector( | |||
| }, | |||
| ( state, rootClientId ) => [ | |||
| state.blockListSettings[ rootClientId ], | |||
| state.editor.present.blocks, | |||
| state.editor.present.blocks.byClientId, | |||
| state.editor.present.blocks.order, | |||
There was a problem hiding this comment.
Nice —this should help a lot! 🔥
There was a problem hiding this comment.
Does order matter for the inserter items? Can we reason differently about nested blocks where it could have an impact? Do I miss anything?
| } | ||
|
|
||
| return flattenedBlockAttributes; | ||
| } |
| @@ -1947,7 +1948,8 @@ export const getInserterItems = createSelector( | |||
| }, | |||
| ( state, rootClientId ) => [ | |||
| state.blockListSettings[ rootClientId ], | |||
| state.editor.present.blocks, | |||
| state.editor.present.blocks.byClientId, | |||
| state.editor.present.blocks.order, | |||
There was a problem hiding this comment.
Does order matter for the inserter items? Can we reason differently about nested blocks where it could have an impact? Do I miss anything?
| @@ -1981,7 +1983,8 @@ export const hasInserterItems = createSelector( | |||
| }, | |||
| ( state, rootClientId ) => [ | |||
| state.blockListSettings[ rootClientId ], | |||
| state.editor.present.blocks, | |||
| state.editor.present.blocks.order, | |||
| state.editor.present.blocks.byClientId, | |||
There was a problem hiding this comment.
The same about the order as above.
|
Closing in favor of #12312 |
Implements #11782 (comment)
I did notice better performance while typing on blocks.
The PR still needs some polish but it's ready to be tested.
cc @sgomes @gziolo