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

Performance: optimize the usage of getBlockIndex #13067

Merged
merged 6 commits into from Jan 25, 2019

Conversation

Projects
None yet
5 participants
@youknowriad
Copy link
Contributor

youknowriad commented Dec 21, 2018

refs #11782 closes #13349

When adding blocks or pressing "Enter", the index of the blocks following the insertion point changes which means all the blocks following this block rerender because they have the order as a prop.

In this PR, I move the getBlockIndex selector usage to the event handlers instead of computing it in withSelect and passing it as a prop to the component to avoid heavy rerenders when pressing "Enter".

@youknowriad youknowriad self-assigned this Dec 21, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Dec 21, 2018

@@ -769,6 +773,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {
} );

export default compose(
pure,

This comment has been minimized.

@youknowriad

youknowriad Dec 21, 2018

Author Contributor

This is useful to avoid rerendering all the blocks when we add/remove a block in the block list (parent component).

I was also tempted to replace it with React.memo as it may be more optimized than our HoC. It would be good to compare performance of these two.

@@ -189,7 +189,6 @@ class BlockList extends Component {
{ map( blockClientIds, ( clientId, blockIndex ) => (
<BlockListBlock
key={ 'block-' + clientId }
index={ blockIndex }

This comment has been minimized.

@youknowriad

youknowriad Dec 21, 2018

Author Contributor

I spent 2 hours trying to find why the blocks were rerendering, it turns out we had a useless prop (that was not even used :P)

@youknowriad youknowriad added this to the 4.9 milestone Dec 21, 2018

isLocked: !! getTemplateLock( rootClientId ),
blocks,
canDuplicate,
rootClientId,
extraProps: props,
};
} ),
withDispatch( ( dispatch, props ) => {
withDispatch( ( dispatch, props, { select } ) => {

This comment has been minimized.

@gziolo

gziolo Dec 22, 2018

Member

@aduth - I don't see it on the list in #12890.

Anyways, I'm happy seeing that select is helping us to improve performance. We still didn't discover any downsides which is encouraging to do more refactoring like this.

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

@aduth - I don't see it on the list in #12890.

I think it was missed because I was explicitly looking for withDispatch( ( dispatch, ownProps ) or withDispatch( ( dispatch, {. We apparently have more variation 😬

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

I think it was missed because I was explicitly looking for withDispatch( ( dispatch, ownProps ) or withDispatch( ( dispatch, {. We apparently have more variation 😬

This is the only occurrence of the second argument being named as "props" for withDispatch, and best I could tell (by a regular expression search of withDispatch\( \( dispatch, [^{o]) there are no other variations.


return {
title: getEditedPostAttribute( 'title' ),
hasItems: hasInserterItems( rootClientId ),
rootClientId,
index,
rootClientId: rootClientId || getBlockInsertionPoint().rootClientId,

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

Should this have been changed from rootClientId, ? There's redundancy here in that the same fallback behavior occurs a few lines above.

const {
__experimentalFetchReusableBlocks: fetchReusableBlocks,
showInsertionPoint,
hideInsertionPoint,
} = dispatch( 'core/editor' );

// eslint-disable-next-line no-restricted-syntax

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

We should have a "Disable reason" if only to clarify what's actually being disabled here, given that no-restricted-syntax is an umbrella rule containing many miscellaneous custom rules.

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Jan 8, 2019

@youknowriad youknowriad force-pushed the update/get-block-index-performance branch from 860fec0 to 94d8884 Jan 21, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 21, 2019

Rebased this, can I get some reviews?

youknowriad added some commits Jan 21, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 22, 2019

Updated to fix the tests and this actually also fixes #13349 :)

@gziolo
Copy link
Member

gziolo left a comment

This is a quite complex PR. I reviewed it but I was not able properly validate changes in packages/editor/src/components/inserter/menu.js file. I wIll try again after I play with PR in the browser.

Everything else seems to be correct. It's hard to follow as there are some changes in the props passed.

return {
isLocked: !! getTemplateLock( rootClientId ),
getClientIdsOfDescendants,
getBlockIndex,

This comment has been minimized.

@gziolo

gziolo Jan 24, 2019

Member

I assume that it isn't something we can handle using withDispatch and select call.

@@ -101,6 +102,7 @@ class BlockDropZone extends Component {
return;
}

const dstIndex = dstClientId ? getBlockIndex( dstClientId, dstRootClientId ) : undefined;

This comment has been minimized.

@gziolo

gziolo Jan 24, 2019

Member

What dst stands for? It would read better if we would avoid such abbreviations.

@aduth

aduth approved these changes Jan 24, 2019

Copy link
Member

aduth left a comment

Looks good. Nice improvements 👍

This was difficult to review because it wasn't easy to associate individual changes with their goal, as there appears to be about a dozen separate goals of this pull request.

getBlockSelectionEnd,
getBlockOrder,
} = select( 'core/editor' );
const { clientId, rootClientId, isAppender } = ownProps;

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

I don't really understand the purpose of this new isAppender prop. Isn't it enough to assume that if the inserter is not provided a clientId and there's no current selection, it should take the appending behavior to the rootClientId?

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

Okay, looking more, I think I understand it a bit better, particularly when the last block is not a paragraph and you have a selection, it's important that the inserter which is shown at the end of content would insert by appending, not after whatever selection happens to exist. The alternative would be that the default appender at the end of content provides the clientId of the last block, but I think the expressiveness of this new prop is preferable.

This comment has been minimized.

@youknowriad

youknowriad Jan 24, 2019

Author Contributor

Also sometimes, there's no block inside a container block with an appender, and the currently selected block is elsewhere on the page, which means the regular flow would insert the new block under the selected block and not inside the container block.

This comment has been minimized.

@youknowriad

youknowriad Jan 24, 2019

Author Contributor

Technically speaking, I'd love to remove the behavior we have where we insert a block after the selected block, it could be considered confusing and there's the in-between inserters to solve the same use-case.

I won't do it in this PR but would love thoughts cc @jasmussen

This comment has been minimized.

@jasmussen

jasmussen Jan 25, 2019

Contributor

Technically speaking, I'd love to remove the behavior we have where we insert a block after the selected block, it could be considered confusing and there's the in-between inserters to solve the same use-case.

Followup question to this: if you used the Add Block button in the top toolbar on a very long page, where would the block be inserted? If the answer is "at the end", it doesn't seem like a good solution. Performance is important, of course, but is there any other way we can do it?

Most word processors insert non-text content where the caret is at. Honestly with the sibling inserter is a new solution with limited prior art. I understand it's a different beast, but we should still not trivially remove this. @alexislloyd what are your thoughts?

This comment has been minimized.

@youknowriad

youknowriad Jan 25, 2019

Author Contributor

I was asking for thoughts and I have them, thanks @jasmussen :)

My impression was that sometimes, the behavior was not well-understood/confusing for some people but I totally agree that the technically matters shouldn't dictate the decision here, as it's not a big drawback to support this.

}

// If there a selected block, we insert after the selected block.
const end = getBlockSelectionEnd();

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

getBlockSelectionEnd is performant enough, but there are optimization opportunities here to avoid calling it when isAppender is true (allow the short-circuiting of the following condition's ! isAppender).

let end;
if ( ! isAppender && ( end = getBlockSelectionEnd() ) ) {
	const selectedBlockRootClientId = getBlockRootClientId( end ) || undefined;
	return {
		index: getBlockIndex( end, selectedBlockRootClientId ) + 1,
		rootClientId: selectedBlockRootClientId,
	};
}

Or:

if ( ! isAppender ) {
	const end = getBlockSelectionEnd();
	if ( end ) {
		const selectedBlockRootClientId = getBlockRootClientId( end ) || undefined;
		return {
			index: getBlockIndex( end, selectedBlockRootClientId ) + 1,
			rootClientId: selectedBlockRootClientId,
		};
	}
}

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

Oh, I forgot this is only called on specific user interactions, not in a withSelect, so not nearly as big a deal.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 24, 2019

Updated to fix the tests and this actually also fixes #13349 :)

I can confirm that it fixes #13349! Nice one :)

@gziolo

gziolo approved these changes Jan 24, 2019

Copy link
Member

gziolo left a comment

In my testing, everything works great in comparison to master. It's even better because of this bug fixed.

I spent time trying to figure out why there is no appender after the last block is a Paragraph with content, but it seems like a design decision. @jasmussen is it explained somewhere?

@youknowriad youknowriad merged commit 4afda93 into master Jan 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/get-block-index-performance branch Jan 25, 2019

daniloercoli added a commit that referenced this pull request Jan 25, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-add-enter-key-detection-to-title

* 'master' of https://github.com/WordPress/gutenberg:
  Add new RSS Block (#7966)
  Performance: optimize the usage of getBlockIndex (#13067)
@designsimply

This comment has been minimized.

Copy link
Contributor

designsimply commented Jan 25, 2019

I spent time trying to figure out why there is no appender after the last block is a Paragraph with content, but it seems like a design decision. @jasmussen is it explained somewhere?

@gziolo perhaps this? #3078 (comment)

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 28, 2019

It's slightly older I think but thanks for checking :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment