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.

Sign up for GitHub

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

Jump to bottom

Avoid rerenders of the entire BlockInspector when block attributes change #21990

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Avoid rerenders of the entire BlockInspector when block attributes change #21990

merged 2 commits into from
Apr 30, 2020

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Apr 30, 2020 •
edited
Loading

Description

BlockInspector uses useSlot( InspectorAdvancedControls.slotName ). Whenever any fill is updated, the entire BlockInspector is re-rendered. This causes performance problems like the ones described in #21973. This PR extracts the InspectorAdvancedControls fills into a sub-component to avoid re-rendering the entire inspector.

How has this been tested?

For some reason, my react dev tools fail to capture the re-render of BlockInspector. This makes testing a little bit harder, but not impossible:

  1. Start without applying this PR
  2. Add console.log("BlockInspector rendered") as a first statement in the BlockInspector component
  3. Open post editor and add a navigation block with some menu items.
  4. Select the entire block and change the background.
  5. Confirm that BlockInspector rendered was logged about 10 times.
  6. Apply this PR and keep the console.log in place
  7. Repeat steps 3-4
  8. Confirm that BlockInspector rendered was not logged in response to changing the background

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamziel adamziel added [Feature] Inspector Controls The interface showing block settings and the controls available for each block [Type] Performance Related to performance efforts labels Apr 30, 2020
@adamziel adamziel requested a review from youknowriad April 30, 2020 11:23
@adamziel adamziel requested a review from ellatrix as a code owner April 30, 2020 11:23
@adamziel adamziel self-assigned this Apr 30, 2020
@adamziel adamziel mentioned this pull request Apr 30, 2020
Merged
6 tasks
@github-actions
Copy link

Size Change: +2 B (0%)

Total Size: 819 kB

Filename Size Change
build/block-editor/index.js 107 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.23 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.11 kB 0 B
build/block-library/editor.css 7.11 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.22 kB 0 B
build/block-library/style.css 7.23 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.05 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.6 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 10.9 kB 0 B
build/edit-site/style-rtl.css 5.11 kB 0 B
build/edit-site/style.css 5.11 kB 0 B
build/edit-widgets/index.js 7.5 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@adamziel
Copy link
Contributor Author

adamziel commented Apr 30, 2020 •
edited
Loading

@youknowriad This PR combined with #21973 fixes the performance issue even without the memo() call. I trust your judgement here and I will remove the memo if you think it's worth it. Just to reiterate my personal opinion: I think memo() is worth having in this specific case. I see it as buying an insurance against accidentally updating <BlockInspector /> in a way that would restore re-renders - our premium is wrapping <BlockStyles /> in memo() . Personally I think it's quite easy to add another hook to BlockInspector without considering the impact on <BlockStyles /> - and this have real UX consequences for one of the core blocks as seen in #21973. At the same time, <BlockStyles/> seems unlikely to become something that's rendered dozens/hundred of times in a single tree so the performance impact of memo() should be marginal, even if it ends up being redundant. Also, before refactoring <BlockStyles /> to use hooks, one of the HOCs still added pure() to the tree - which means even with memo() we're still net neutral with regard to the amount of pure components.

@youknowriad
Copy link
Contributor

@adamziel Thanks for your work, these improvements are great. I'll take a look at this PR a bit later but keeping "memo" is totally fine.

Good point about BlockStyles being expensive, that said, I wonder if the expensive component is actually BlockPreview used by BlockStyles and not BlockStyles itself so I wonder whether we should move the "memo" there.

@adamziel
Copy link
Contributor Author

Thank you!

I wonder if the expensive component is actually BlockPreview used by BlockStyles and not BlockStyles itself so I wonder whether we should move the "memo" there.

@youknowriad Good point! I'll merge #21973 and spin a new PR for that.

youknowriad
youknowriad approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I didn't really test the performance impact here but the changes look good regardless.

@adamziel adamziel mentioned this pull request Apr 30, 2020
Merged
6 tasks
@adamziel
Copy link
Contributor Author

adamziel commented Apr 30, 2020 •
edited
Loading

Awesome, thank you so much! Also, that new PR to memo() the BlockPreview is available here: #21993

@adamziel adamziel merged commit 97caac7 into master Apr 30, 2020
@adamziel adamziel deleted the fix/block-inspector-rerenders branch April 30, 2020 12:45
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

Assignees

@adamziel adamziel

Labels
[Feature] Inspector Controls The interface showing block settings and the controls available for each block [Type] Performance Related to performance efforts
Projects
None yet
Milestone
Gutenberg 8.1
Development

Successfully merging this pull request may close these issues.
2 participants
@adamziel @youknowriad