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

Improve performance; Stop unnecessary rerenders caused by withColors #6686

Merged
merged 1 commit into from May 11, 2018

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented May 10, 2018

The getColor and setColor functions returned a new object or a new function every time, although the code looked correct it made unnecessary rerenders happen.

We now return memoized functions this ensures that if the parameters are not changed we will return references for the same object or function so the props the HOC returns are not changed and no rerender happen.

How has this been tested?

Verify setting colors in paragraph and button continues to work the same way as before.
Toggle the Highlights updates feature and verify, and verify the number of rerenders decreased.

Screenshots

Before:
may-10-2018 14-51-48
After:
may-10-2018 14-49-49

@jorgefilipecosta jorgefilipecosta self-assigned this May 10, 2018
@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core May 11, 2018
@gziolo gziolo requested a review from aduth May 11, 2018
@gziolo gziolo added this to the 2.9 milestone May 11, 2018
return setColorValue( colors, colorNameAttribute, customColorAttribute, setAttributes );
};
return mapGetSetColorToProps( getColor, setColor, props );
return mapGetSetColorToProps( memoizedGetColor( colors ), memoizedSetColor( props.setAttributes )( colors ), props );

This comment has been minimized.

Copy link
@gziolo

gziolo May 11, 2018

Member

props.setAttributes - out of curiosity, why would it ever change?

Copy link
Member

tofumatt left a comment

Seems like a straightforward change that makes sense to me.

I guess I share @gziolo's question about why props.setAttributes would change, but the approach here looks good and causes fewer re-renders 👍

@jorgefilipecosta jorgefilipecosta force-pushed the update/improved-performance-withColors branch from 2721e3d to e45ac23 May 11, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented May 11, 2018

Hi @tofumatt and @gziolo thank you for the reviews. I added a comment in the code clarifying why setAttributes was memoized.

Copy link
Member

tofumatt left a comment

Thanks, looks great!

Could you make a minor change to the comment before merging? I just found the current one a bit hard to parse.

/**
* Even though, we don't expect setAttributes to change memoizing it is essential.
* If it is not memoized, each time memoizedSetColor is called,
* even if setAttributes was the same as before a new function reference is returned.

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 11, 2018

Member

Nitpick here but this is hard to parse. Could you modify the sentence a bit so it's easier to follow cause-and-effect, eg:

* If setAttributes is not memoized, each time memoizedSetColor is called:
* a new function reference is returned (even if setAttributes has not changed).

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 11, 2018

Author Member

Thank you for the improvements they were applied 👍

The getColor and setColor functions returned a new object or a new function every time, although the code looked correct it made unnecessary rerenders happen. We now returned memoized functions this ensure that if the parameters are not changed we will return references for the same object or function so no rerender happen.
@jorgefilipecosta jorgefilipecosta force-pushed the update/improved-performance-withColors branch from e45ac23 to 56a11cb May 11, 2018
@jorgefilipecosta jorgefilipecosta merged commit 7fb242b into master May 11, 2018
2 checks passed
2 checks passed
codecov/project 44.09% (+0.03%) compared to 5f5ae54
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jorgefilipecosta jorgefilipecosta deleted the update/improved-performance-withColors branch May 11, 2018
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { get } from 'lodash';
import { get, memoize } from 'lodash';

This comment has been minimized.

Copy link
@gziolo

gziolo May 11, 2018

Member

We use memize library created by our own @aduth, which is much more performant.

This comment has been minimized.

Copy link
@tofumatt

tofumatt May 11, 2018

Member

Thanks, I've filed #6707 as a follow-up.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 11, 2018

Author Member

Thank you @gziolo for referring the lib and thank you for the rule @tofumatt. Unfortunately, I was not aware of this lib. I will use it in the future.

* a new function reference is returned (even if setAttributes has not changed).
* This would make our memoized chain useless.
*/
const memoizedSetColor = memoize(

This comment has been minimized.

Copy link
@gziolo

gziolo May 11, 2018

Member

3 nested memoize calls look suspicious in here. Maybe it's okey, just saying :)

This comment has been minimized.

Copy link
@aduth

aduth May 11, 2018

Member

Also, why would we want to preserve cache references to old setAttributes ? Seems like a use-case for memoize-one or a WeakMap-based cache like the withWeakMapCache proposed in #6395.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 11, 2018

Author Member

Unfortunately, memoize-one would not fit our use case, as each block has its setAttributes function and we would need to cache it for this approach to work.

I had the idea in mind that when a block was deleted, we would stop referencing the memoized function which would make its cache clear. But unfortunately given that I made the memoize call a top-level function, that is not true. So when blocks are deleted, we are still catching its deleted setAttributes :( Using WeakMap caching it would make the logic correct without leaks I think.
But anyway I got mind bugged and this solution is complex and probably not something we should pursue in the long term.

At the time using memoize seemed the best possibility to solve the rerender problem being back-compatible, now it seems the best long-term solution is changing the API. I'm still a little bit curious about how the rerenders could be solved keeping the same API without a WeakMap (and the preservation of references in deleted blocks) it seems there is a simple way and I'm not seeing it but now it is more an academic curiosity and not something I would pursue in the long term.

Thank you for your reviews I will follow up with them to address this problem.

@@ -14,6 +14,40 @@ import { withSelect } from '@wordpress/data';
*/
import { getColorValue, getColorClass, setColorValue } from './utils';

const memoizedGetColor = memoize(
( colors ) =>

This comment has been minimized.

Copy link
@aduth

aduth May 11, 2018

Member

There's a potential memory leak here when getEditorSettings().colors is not defined. In the withSelect below, you pass [] as the defaultValue in the get call. Since [] !== [], these will be stored as separate entries in the memoization cache.

$ n_
n_ > var fn = () => console.log( 'called' );
undefined
n_ > var memoized = _.memoize( fn );
undefined
n_ > memoized( [] );
called
undefined
n_ > memoized( [] );
called
undefined
n_ > memoized.cache.size
2

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 11, 2018

Author Member

Nice catch 👍

@aduth

This comment has been minimized.

Copy link
Member

aduth commented May 11, 2018

Why do we leave it to the block implementer to provide a mapping function? Particularly when I imagine these prop names would never change. Since it all seems to be based around the underlying component receiving a prop of values, I wonder if we could just accept this name as a sole argument of withColors. As a component, we could also effect the cache behavior via shouldComponentUpdate. Something like:

const DEFAULT_COLORS = [];

export const withColors = ( propName ) => createHigherOrderComponent(
	( WrappedComponent ) => {
		const upperFirstName = upperFirst( propName );
		const attributeName = propName;
		const customAttributeName = 'custom' + upperFirstName;
		const setterPropName = 'set' + upperFirstName;
		const contextName = kebabCase( attributeName );

		class WithColorsComponent extends Component {
			constructor() {
				super( ...arguments );

				this.setColor = this.setColor.bind( this );
			}

			shouldComponentUpdate( nextProps ) {
				const { attributes } = this.props;
				const { attributes: nextAttributes } = nextProps;
				return (
					attributes[ attributeName ] !== nextAttributes[ attributeName ] ||
					attributes[ customAttributeName ] !== nextAttributes[ customAttributeName ]
				);
			}

			setColor( value ) {
				const { colors, setAttributes } = this.props;
				setColorValue( colors, attributeName, customAttributeName, setAttributes );
			}

			render() {
				const { colors, attributes } = this.props;
				const color = attributes[ attributeName ];
				const customColor = attributes[ customAttributeName ];

				return (
					<WrappedComponent
						{ ...this.props }
						{ ...{
							[ attributeName ]: {
								name: color,
								class: getColorClass( contextName, color ),
								value: getColorValue( colors, color, customColor ),
							},
							[ setterPropName ]: this.setColor,
						} }
					/>
				);
			}
		}

		return withSelect( ( select ) => {
			const { getEditorSettings } = select( 'core/editor' );
			const colors = get( getEditorSettings(), [ 'colors' ], DEFAULT_COLORS );
			return { colors };
		} );
	},
	'withColors'
);

Then paragraph block uses as:

compose( [
	withColors( 'color' ),
	withColors( 'backgroundColor' ),
] )( ParagraphBlock )
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented May 12, 2018

Hi @aduth, thank you for your suggestions.
I have been thinking about simplifying things with an API change. And we still have some challenges.
I think we can not compare the attributes in the shouldComponentUpdate because it would make paragraph only rerender if attributeName or customAttributeName change ignoring all other attribute changes.
For example, if I add this to paragraph block:

	edit: compose(
		createHigherOrderComponent(
			( WrappedComponent ) => {
				return class testHOC extends Component {
					shouldComponentUpdate( nextProps ) {
						return (
							this.props.attributes.textColor !== nextProps.attributes.textColor
						);
					}

					render() {
						return (
							<WrappedComponent
								{ ...this.props }
							/>
						);
					}
				};
			},
			'testHOC'
		),
		...

It only rerenders when textColor changes.
I think one option could be including textColor/backgroundColor in the HOC state. Then we can compare it against the new props in getDerivedStateFromProps. If they are different we regenerate the state including name-class-value object and the setter if they are equal we don't change the state. But saving the props in the state just because of comparing against the new props does not seems correct.
Another option is make the prop comparison in shouldComponentUpdate and if they are different we would regenerate new object and setter part of components e.g: this.colorObj = { name: ... }. In the render, we would always pass the object colorObj. But that would make shouldComponentUpdate impure as it would be changing an object part of the component (so it is an even worst solution when compared with getDerivedStateFromProps ).
Offcourse using componentWillReceiveProps makes this problem trivial but that is not an option now.

I'm probably missing something and complicating things, I will keep thinking about a solution.

Edit:
It seems like using getDerivedStateFromProps and saving references of the props we want to check for changes in the state is the correct approach reactjs/reactjs.org#721 (comment). React will stop keeping references to previous props so if we need to check for changes we should save them in the state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.