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.

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 performance of global inserter: don't rerender on every attribute change. #6796

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented May 17, 2018

Inserter withSelect passed many props that contain objects that change on every block attribute change e.g.: getBlocks. These props are used only on withDispatch. We changed the withSelect not to pass complete objects but just pass the properties that in fact, withDispatch uses.

How has this been tested?

Verify the global inserter still works as expected.
Use the highlight updates browser feature to check it does not rerender on every attribute change e.g: key press in a paragraph.

Screenshots

Before:
may-17-2018 10-58-49
After:
may-17-2018 10-56-45

…bute change e.g: each character in a paragraph.

Inserter withSelect passed many props that contain objects that change on each block attribute change eg: getBlocks. This props were only used on withDispatch. We change the withSelect to not pass all the objects but just pass the properties that in fact withDispatch uses.
@jorgefilipecosta jorgefilipecosta self-assigned this May 17, 2018
@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts labels May 17, 2018
@jorgefilipecosta jorgefilipecosta requested a review from a team May 17, 2018 14:34
}
return dispatch( 'core/editor' ).insertBlock( insertedBlock, index, rootUID );
return dispatch( 'core/editor' ).insertBlock( insertedBlock, insertionPointIndex, insertionPointRootUID );
Copy link
Contributor

@youknowriad youknowriad May 17, 2018

Choose a reason for hiding this comment

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

I have memories of @aduth having a PR to refactor this using effects, but I'm not certain 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

I have memories of @aduth having a PR to refactor this using effects, but I'm not certain 🤷‍♂️

Refactor which? I have dreams (and local in-progress branches) of a world without explicit index and root UID arguments, though they have yet to be realized in the form of a pull request. I might have similar ones though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for clarifying this @aduth. In that case, do you think the changes here make sense or you think it is better to wait/work on bigger changes that remove the need for index and root uid arguments?

@aduth
Copy link
Member

aduth commented May 25, 2018

do you think the changes here make sense

I think there's value. I've certainly noticed the flickering Inserter button when React updates are highlighted while typing. I'd guess the overall impact is small: AFAICT it should only be the top-level Inserter component which is reconciling itself, not any of its descendants?

An alternative thought I'd had previously was maybe there'd be benefit in a higher-order component which omits certain props, since in cases like this (and others), we're injecting props which are only used in composition (selectedBlock from withSelect to withDispatch, but used no further).

See also: Recompose documentation mention of an omitProps implementation

I'm also a bit curious the cost comparison of calling isUnmodifiedDefaultBlock on every state change where a block is selected, which was not the case previously. It's not particularly trivial for the majority case where we're typing within a paragraph:

export function isUnmodifiedDefaultBlock( block ) {
const defaultBlockName = getDefaultBlockName();
if ( block.name !== defaultBlockName ) {
return false;
}
const newDefaultBlock = createBlock( defaultBlockName );
const attributeKeys = applyFilters( 'blocks.isUnmodifiedDefaultBlock.attributes', [
...keys( newDefaultBlock.attributes ),
...keys( block.attributes ),
] );
return every( attributeKeys, ( key ) =>
isEqual( newDefaultBlock.attributes[ key ], block.attributes[ key ] )
);
}

@jorgefilipecosta
Copy link
Member Author

I'm also a bit curious the cost comparison of calling isUnmodifiedDefaultBlock on every state change where a block is selected, which was not the case previously. It's not particularly trivial for the majority case where we're typing within a paragraph.

That's, in fact, a disadvantage of this approach. I'm closing this PR as we are not certain if, in fact, it increases the performance.

@jorgefilipecosta jorgefilipecosta deleted the fix/remove-inserter-rerenders-on-each-attribute-change branch June 26, 2018 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement. [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants