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 isEditedPostEmpty selector #13086

Merged
merged 4 commits into from Jan 8, 2019

Conversation

Projects
None yet
3 participants
@youknowriad
Copy link
Contributor

youknowriad commented Dec 24, 2018

Refs #11782

The isEditedPostEmpty is a selector called on each key stroke, so optimizing it has an impact on the feeling of smoothness while typing in long posts, in this PR I avoid using the slow getBlocks selector in the isEditedPostEmpty selector.

Noting that I'm making this optimization because when I tried merging a lot of pending performance PRs together, this selector was taking 1/2 of the key strike time in a long post.

In the current state of master, I'm noticing a 13% improvement while typing but I expect this number to be way bigger once all the other PRs are merged.

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

@youknowriad youknowriad self-assigned this Dec 24, 2018

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

@tofumatt
Copy link
Member

tofumatt left a comment

Nice 👍

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 2, 2019

I'd expect this to introduce some issue, specifically toward the special handling provided through getBlocksForSerialization for posts which consist of only one empty (unmodified) paragraph block. I guess that's handled by the fact that allowing getEditedPostContent to run would itself generate getBlocksForSerialization and produce the empty (thus falsey) result?

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 3, 2019

I'd expect this to introduce some issue, specifically toward the special handling provided through getBlocksForSerialization for posts which consist of only one empty (unmodified) paragraph block. I guess that's handled by the fact that allowing getEditedPostContent to run would itself generate getBlocksForSerialization and produce the empty (thus falsey) result?

To be honest, I forgot this use-case but yeah it seems that it would work properly because of the fallback. And I think it's a fine drawback because we want to optimize the call when its cost is high (when it has a lot of blocks) while in this case it only contains a single block so the fallback will still be fast.

@aduth
Copy link
Member

aduth left a comment

I agree that I think in practice this should always work, based on the current logic of getBlocksForSerialization. That said, it very much ties this selector to that specific implementation. We should at least very clearly document this. An alternative might be to have some separate getBlockClientIdsForSerialization for the purpose of avoiding the call to getBlocks while still respecting the logic of a "for serialization" condition.

@@ -454,18 +454,18 @@ export function isEditedPostSaveable( state ) {
* @return {boolean} Whether post has content.
*/
export function isEditedPostEmpty( state ) {
const blocks = getBlocksForSerialization( state );
const rootBlocks = getBlockOrder( state );

This comment has been minimized.

@aduth

aduth Jan 3, 2019

Member

It'd be more accurate to describe this as a set of client IDs, not blocks, i.e. rootClientIds.

@youknowriad youknowriad force-pushed the update/optimize-is-empty-post branch from 470b394 to fded801 Jan 4, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 7, 2019

@aduth Feel free to update to document properly, I'm not certain I understand exactly what comment you want to add.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 7, 2019

What are your thoughts about a getBlockClientIdsForSerialization selector? My main reservation is that, as implemented, we have a fragile assumption taking place in isEditedPostEmpty.

Pro getBlockClientIdsForSerialization:

  • Not prone to same fragility
  • Can be optimized nearly just as well, since getBlocksForSerialization only does anything "special" if there's a single block in the post, which can be determined by a trivial getBlockOrder function call prior to inspecting the full block object

Con getBlockClientIdsForSerialization:

  • Getting into "excessively specific" selector territory
  • Still has slightly more overhead than as considered here
  • isEditedPostEmpty already does some bad practice by assuming internals, what's a bit more? 😄
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 7, 2019

Getting into "excessively specific" selector territory

This is my concern. I'm not too worried about selectors assuming internals as selectors are very related to the state shape which is also an internal. That said I'm not strongly against getBlockClientIdsForSerialization. We could add it without exposing it but we'd still need a getBlock call to check isUnmodifiedDefaultBlock if the block count is 1, so not great either.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 7, 2019

but we'd still need a getBlock call to check isUnmodifiedDefaultBlock if the block count is 1, so not great either.

Yeah, the thought is that if there's only one block, we're nowhere near the edge case of degraded performance which this pull request primarily impacts.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 7, 2019

In adding some unit tests here, I found that the current logic actually fails to account for getBlocksForSerialization, specifically in that the following condition will wrongly return false for an unmodified default block, rather than the true value the function is expected to return.

if ( getBlockName( state, rootClientIds[ 0 ] ) !== getFreeformContentHandlerName() ) {
return false;
}

@aduth

aduth approved these changes Jan 7, 2019

Copy link
Member

aduth left a comment

Pushed some changes to fix the aforementioned issue, expand on inline comments, and add relevant unit tests.

Feel free to merge if it looks okay to you.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 8, 2019

Thanks for the update Andrew, appreciate it. These changes look good to me.

@youknowriad youknowriad merged commit cded0f9 into master Jan 8, 2019

1 check passed

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

@youknowriad youknowriad deleted the update/optimize-is-empty-post branch Jan 8, 2019

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