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

Block Editor: Flatten, shortcut align hook rendering if unaligned #18963

Merged
merged 1 commit into from Dec 6, 2019

Conversation

@aduth
Copy link
Member

aduth commented Dec 6, 2019

This pull request seeks to optimize the implementation of the block alignment BlockListBlock filtered higher-order component.

It seeks to improve performance in two ways:

  • Flatten the component hierarchy by refactoring the compose flow of withSelect and an inner component, to a single component using useSelect.
  • Shortcuts the rendering early if the block does not have an assigned align attribute, in order to skip logic involved in validating alignment value (skips calls to getValidAlignments, getBlockSupport, hasBlockSupport, and an includes iteration on the validAlignments result).

The aim here is to optimize that the majority of blocks will not have an alignment value, and can skip this hook logic.

There might be some way to optimize this even further:

  • If we could detect the absence of an attribute even earlier than when the useSelect hook is rendered, to avoid another store subscription. Since the hook cannot be conditionally rendered, this might have to be part of a separate component or higher-order component, which loses some of the benefit in flattening the hierarchy.
  • Move the logic to be handled directly in BlockEditBlock, rather than filtering at all. The advantage of the filtering here is in colocating all of the alignments behavior, but there may be an alternative approach we could consider which doesn't have the same overhead as the filter.

Testing Instructions:

Verify there are no regressions in the rendering of a block, notably on valid alignments of a block, e.g. in themes with and without wide alignment support (should not show as wide-aligned in the editor if the theme doesn't support it).

Ensure unit tests pass:

npm run test-unit packages/block-editor/src/hooks/test/align.js
Copy link
Contributor

youknowriad left a comment

This is a great refactoring. Were you able to measure any performance impact?

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 6, 2019

@youknowriad I was having some difficulty with the performance test suite yesterday, but I was initially drawn to this code from a Chrome DevTools Performance recording, so there's some space for gains, likely in the 0-3ms range per keypress. I'll see if I can get the tests working today.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 6, 2019

I'm still not entirely sure the performance tests are cooperating (and the results are a bit all over the place), but for what I do see, it doesn't seem like this is having too much of an impact for better or worse:

Before:

Average time to load: 4382ms
Average time to DOM content load: 4111ms
Average time to type character: 67.425ms
Slowest time to type character: 139ms
Fastest time to type character: 39ms

After:

Average time to load: 4190ms
Average time to DOM content load: 3915ms
Average time to type character: 67.39ms
Slowest time to type character: 136ms
Fastest time to type character: 38ms

Conceptually, I can't imagine how it could be anything other than an improvement, based on the proposed advantages here. At the very least it could be seen as as a useful refactoring to simplify the implementation of this behavior, and toward consistency of hooks usage and component flattening.

@aduth aduth merged commit 454239c into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the update/perf-align branch Dec 6, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 6, 2019

@aduth if you want to push the performance tests further, you can try to disable the "Async Mode" which "virtually" improves performance and see if there's impact without it. (I don't think it's necessary though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.