Optimize BlockStyles by using hooks and React.memo (instead of HOCs) #21973

adamziel · 2020-04-29T13:43:20Z

Description

BlockStyles depend on the value of a block, which means they're re-rendered every time an attribute is updated. This is pretty slow, especially if it happens after every key stroke (see #21948). This PR leverages React.memo and useSelect to avoid re-renders in cases where block example is available.

Test plan

To test the case where example is available, create a navigation block.
Select your new block, open block inspector, change style a few times, change background color, confirm the styles are rendered correctly.
To test the case where example is available, manually remove the example from packages/block-library/src/navigation/index.js and repeat previous steps. I think all core blocks come with an example so it seems to be the easiest way to test. In this scenario, confirm that the style preview is updated to reflect all the updates to block attributes - e.g. changing background color should be reflected in the preview right away.

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.

youknowriad · 2020-04-29T13:47:15Z

packages/block-editor/src/components/block-styles/index.js

@@ -7,6 +7,8 @@ import classnames from 'classnames';
 /**
 * WordPress dependencies
 */
+import isShallowEqual from '@wordpress/is-shallow-equal';
+import { memo } from '@wordpress/element';


noting that we also have a "pure" hoc which is the same thing (before the existence of memo), not sure if we should deprecate it or not.

I also find both these hocs to be less and less useful with hooks

I have no strong preference here - I'm fine with just memo, but if you think it would be cleaner with pure I'll update it.

github-actions · 2020-04-29T13:49:36Z

Size Change: +49 B (0%)

Total Size: 816 kB

Filename	Size	Change
`build/block-editor/index.js`	106 kB	+44 B (0%)
`build/components/index.js`	179 kB	+5 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.03 kB	0 B
`build/block-library/editor.css`	7.03 kB	0 B
`build/block-library/index.js`	114 kB	0 B
`build/block-library/style-rtl.css`	7.14 kB	0 B
`build/block-library/style.css`	7.14 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/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`	3.54 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}

youknowriad

I have trouble understanding why we need "memo" here since for me withSelect/useSelect should already not rerender if you don't include the "block" prop (with the updated code)?

youknowriad · 2020-04-29T13:50:59Z

Some random questions :)

how did you test performance impact?
did you consider rewriting with useSelect/useDispatch?

adamziel · 2020-04-29T14:05:58Z

I have trouble understanding why we need "memo" here since for me withSelect/useSelect should already not rerender if you don't include the "block" prop (with the updated code)?

I believe withSelect returns a pure component, but does not make WrappedComponent pure. If parent component passes the same props, React will not re-render. If the callback returns the same props, React will still re-render:

gutenberg/packages/data/src/components/with-select/index.js

Lines 50 to 60 in 4857ad5

    
           const withSelect = ( mapSelectToProps ) => 
        
           	createHigherOrderComponent( 
        
           		( WrappedComponent ) => 
        
           			pure( ( ownProps ) => { 
        
           				const mapSelect = ( select, registry ) => 
        
           					mapSelectToProps( select, ownProps, registry ); 
        
           				const mergeProps = useSelect( mapSelect ); 
        
           				return <WrappedComponent { ...ownProps } { ...mergeProps } />; 
        
           			} ), 
        
           		'withSelect' 
        
           	);

how did you test performance impact?

Nothing overly scientific :-) During my work on #21948 I noticed typing in block inspector is super slow. 60 FPS video would be much better medium to show the problem, but let's try with GIF:

Without the PR the slowness is impossible to ignore - letters are not showing immediately in response to my input, and they often appear in chunks of 2-6.

did you consider rewriting with useSelect/useDispatch?

I did not! Hmm.. useSelect would receive new block after every key stroke too so it sounds like this approach would still involve memo/pure at some point?

youknowriad · 2020-04-29T14:09:41Z

I believe withSelect returns a pure component, but does not make WrappedComponent pure. If parent component passes the same props, React will not re-render. If the callback returns the same props, React will still re-render:

🤔 withSelect uses useSelect which IIRC doesn't rerender if the callback returns the same props (at least it's supposed to not rerender)

adamziel · 2020-04-29T14:23:55Z

🤔 withSelect uses useSelect which IIRC doesn't rerender if the callback returns the same props (at least it's supposed to not rerender)

I tried removing the memo() and keeping the other changes in tact and it did the trick, nice - thank you! I'll play with it a bit to leverage useSelect/useDispatch instead of HOCs.

adamziel · 2020-04-29T14:50:48Z

@youknowriad I replaced HOC with hooks (useSelect/useDispatch). In the process, we lost the pure() call from withSelect so I ended up wrapping the export in memo() anyway.

youknowriad · 2020-04-29T14:57:06Z

Thanks for the updates @adamziel

Do you happen to know why the wrapping in "memo" is needed here? I mean the issue could probably be on a parent component rerendering where it shouldn't?

youknowriad

Code looks good to me, Ideally we get rid of the "memo" usage too if possible.

adamziel · 2020-04-30T09:52:40Z

@youknowriad that's exactly the case. I would consider it a related, but separate problem. I am profiling right now and if the fix is simple enough I may be able to propose another PR today. I still think we should use memo() here - it costs almost nothing and provides a nice boundary as in "regardless of what happened in the entire tree above, this costly component won't re-render unnecessarily". Without memo(), it would be easy to change something in a seemingly unrelated place and unknowingly break performance in here.

adamziel · 2020-04-30T09:53:44Z

Or am I missing something? Is there any downside to using memo() as long as we're careful not to use it everywhere?

youknowriad · 2020-04-30T10:15:23Z

@adamziel With the number of components rendered in the tree, memoization has costs, if it's on a component that is rendered very high in the tree, it has almost no cost but the more we go down, the more costful it can be because the same component can be rendered in a loop or something. I think also memoization can hide some problems that need solving so I personally prefer avoiding it if possible.

adamziel · 2020-04-30T11:24:02Z

@youknowriad In principle I agree. In this very specific instance, I still think memo() is justified. This component is indeed pretty deep in the tree, but it's an expensive one - it creates a pretty large, dynamic/arbitrary tree of it's own. It seems unlikely to become a basic component that's instantiated in hundreds/thousands times within a single tree.

As for hiding other problems, I fully agree. One such problem I've seen is that these style preview are full of RichText instances and other edit-specific components. It's not a terrible problem, but ideally they would only contain bare minimum amount of components necessary to display the preview.

Also, I think I found the reason behind the parent component being re-rendered. Let's continue the discussion in #21990

Use React.memo() in BlockStyles

254d414

adamziel added [Feature] Inspector Controls The interface showing block settings and the controls available for each block [Type] Performance Related to performance efforts [Feature] Theme Style Variations Related to style variations provided by block themes labels Apr 29, 2020

adamziel requested review from draganescu, youknowriad, talldan, gziolo and ellatrix April 29, 2020 13:43

adamziel self-assigned this Apr 29, 2020

adamziel mentioned this pull request Apr 29, 2020

Make parts of the BlockNavigationList overridable using slots #21948

Merged

6 tasks

youknowriad reviewed Apr 29, 2020

View reviewed changes

Replace HOC with hooks

133d863

youknowriad approved these changes Apr 30, 2020

View reviewed changes

adamziel mentioned this pull request Apr 30, 2020

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

Merged

6 tasks

adamziel changed the title ~~Use React.memo() in BlockStyles~~ Optimize BlockStyles by using hooks and React.memo (instead of HOCs) Apr 30, 2020

adamziel merged commit daa2154 into master Apr 30, 2020

adamziel deleted the fix/slow-block-preview branch April 30, 2020 12:18

github-actions bot added this to the Gutenberg 8.1 milestone Apr 30, 2020

This was referenced Apr 30, 2020

[Optimize] Move memo() from BlockStyles to BlockPreview #21993

Merged

Refactor <BlockStyles /> - precompute preview blocks using an effect #22133

Closed

adamziel mentioned this pull request May 8, 2020

Allow editing of new menu items from the block inspector #22210

Merged

6 tasks

adamziel mentioned this pull request May 14, 2020

More accurate dependency list for useGenericPreviewBlock hook #22355

Merged

6 tasks

adamziel mentioned this pull request Jun 3, 2020

Styles cause lag on Block insertion and selection #22823

Closed

ocean90 mentioned this pull request Jul 26, 2020

The "Default" style is gone from WordPress 5.5. #24147

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize BlockStyles by using hooks and React.memo (instead of HOCs) #21973

Optimize BlockStyles by using hooks and React.memo (instead of HOCs) #21973

adamziel commented Apr 29, 2020 •

edited

Loading

youknowriad Apr 29, 2020

youknowriad Apr 29, 2020

adamziel Apr 29, 2020 •

edited

Loading

github-actions bot commented Apr 29, 2020 •

edited

Loading

youknowriad left a comment

youknowriad commented Apr 29, 2020

adamziel commented Apr 29, 2020 •

edited

Loading

youknowriad commented Apr 29, 2020

adamziel commented Apr 29, 2020

adamziel commented Apr 29, 2020

youknowriad commented Apr 29, 2020

youknowriad left a comment

adamziel commented Apr 30, 2020 •

edited

Loading

adamziel commented Apr 30, 2020

youknowriad commented Apr 30, 2020

adamziel commented Apr 30, 2020

Optimize BlockStyles by using hooks and React.memo (instead of HOCs) #21973

Optimize BlockStyles by using hooks and React.memo (instead of HOCs) #21973

Conversation

adamziel commented Apr 29, 2020 • edited Loading

Description

Test plan

Types of changes

Checklist:

youknowriad Apr 29, 2020

Choose a reason for hiding this comment

youknowriad Apr 29, 2020

Choose a reason for hiding this comment

adamziel Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 29, 2020 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 29, 2020

adamziel commented Apr 29, 2020 • edited Loading

youknowriad commented Apr 29, 2020

adamziel commented Apr 29, 2020

adamziel commented Apr 29, 2020

youknowriad commented Apr 29, 2020

youknowriad left a comment

Choose a reason for hiding this comment

adamziel commented Apr 30, 2020 • edited Loading

adamziel commented Apr 30, 2020

youknowriad commented Apr 30, 2020

adamziel commented Apr 30, 2020

adamziel commented Apr 29, 2020 •

edited

Loading

adamziel Apr 29, 2020 •

edited

Loading

github-actions bot commented Apr 29, 2020 •

edited

Loading

adamziel commented Apr 29, 2020 •

edited

Loading

adamziel commented Apr 30, 2020 •

edited

Loading