Components: Optimize withFilters, avoiding per-instance bindings #13231

aduth · 2019-01-07T21:45:14Z

Related: #12824

This pull request seeks to optimize the implementation of withFilters. In my testing of a large post consisting of 20,000 words (70 headings, 501 paragraphs), ~400ms real-time is spent during initialization in addHook, runHooks, and FilteredComponent#constructor. These changes bring this down to roughly 170ms, with runHooks being the primary contributor (160ms), as it's also used extensively elsewhere in blocks parsing (i.e. withFilters contribution approaches negligible).

Before	After

Implementation notes:

The specific optimizations include:

Binding at most one hook handler for addFilter and removeFilter per hook name. Previously, a handler was added for each instance of a filtered component.
Creating a single shared filtered component definition per hook name. Previously, each instance would create their own filtered component.

There has been one implementation change: Previously, the hooks were bound in the component's constructor. It has been changed here to occur during componentDidMount. This is in accordance with React recommendations, and avoids potential issues with lingering subscriptions in a server-rendered environment (see also #11819).

Avoid introducing any side-effects or subscriptions in the constructor. For those use cases, use componentDidMount() instead.

https://reactjs.org/docs/react-component.html#constructor

Testing instructions:

Repeat testing instructions from #4428, substituting the hook name blocks.BlockEdit with the since-renamed editor.BlockEdit. In other words, insert this code in your console:

wp.hooks.addFilter( 'editor.BlockEdit', 'my/filters', () => () => wp.element.createElement( 'h1', {}, 'My block' ) );

aduth · 2019-01-07T21:49:44Z

packages/components/src/higher-order/with-filters/index.js

-					this.Component = applyFilters( hookName, OriginalComponent );
-					this.forceUpdate();
-				}, ANIMATION_FRAME_PERIOD );
+				this.throttledForceUpdate = debounce(


We could probably go one step further and move the debounce to a single shared function, calling each instance's forceUpdate directly. This has a couple advantages:

The constructor can be dropped altogether

Avoids need to cancel in componentWillUnmount

Automatically accounts for unmounting of components, since the callback would only evaluate FilteredComponentRenderer.instances after the debounce delay

Since it requires a bit more reorganization, I'm a bit inclined to leave this for a subsequent pull request.

Yes, this sounds like a good idea.

gziolo

There are some failing e2e tests which look related to this change:

annotations
block alignment

I think they use withFilters behind the scenes. It should be further investigated.

gziolo · 2019-01-08T07:10:30Z

packages/components/src/higher-order/with-filters/index.js

-				addAction( 'hookAdded', this.namespace, this.onHooksUpdated );
+				// If there were previously no mounted instances for components
+				// filtered on this hook, add the hook handler.
+				if ( FilteredComponentRenderer.instances.length === 1 ) {


This might trigger twice:

when incrementing the number of instances

when decrementing the number of instances

It should only be executed when the number of instances changes from 0 to 1.

Thinking a bit more about it, it is probably correct. I mean, that when mounting it always increments the counter.

Thinking a bit more about it, it is probably correct. I mean, that when mounting it always increments the counter.

Yes, being that it only increments here, I think it should be fine as it is.

gziolo · 2019-01-08T07:15:19Z

packages/components/src/higher-order/with-filters/index.js

-				this.Component = applyFilters( hookName, OriginalComponent );
-				this.namespace = uniqueId( 'core/with-filters/component-' );
-				this.throttledForceUpdate = debounce( () => {
-					this.Component = applyFilters( hookName, OriginalComponent );


Unless I'm missing something, if this is meant to account for new filters applied after the initial render, isn't it missing a critical piece of calling applyFilters again after those new filter handlers are added / removed?

@aduth, I saw this question in my inbox. I can't see it anymore in the original PR. I just wanted to confirm that it is triggered here and in your optimized version properly.

The benefit of calling applyFilters inside of the throttledForceUpdate method is that it will be executed only once when multiple calls of add/remove filters occurs in the same small period of time.

@aduth, I saw this question in my inbox. I can't see it anymore in the original PR. I just wanted to confirm that it is triggered here and in your optimized version properly.

Yes, I deleted it once I noticed I'd overlooked the re-assignment in the debounce callback.

The benefit of calling applyFilters inside of the throttledForceUpdate method is that it will be executed only once when multiple calls of add/remove filters occurs in the same small period of time.

Yeah, I can see that being a good thing. It might require that we do the refactoring I'd pondered at #13231 (comment) ?

gziolo · 2019-01-08T07:21:27Z

packages/components/src/higher-order/with-filters/index.js

+			}
+
+			// Recreate the filtered component.
+			FilteredComponent = applyFilters( hookName, OriginalComponent );


I think this logic can be moved to instance.throttledForceUpdate();. At the moment it's going to be called every time a hook is added or removed. By moving it to the throttled call we will ensure that it happens only once per every re-render. The easiest way to test it is to add 2 or 3 filters at the same time.

Moving this back to throttledForceUpdate would mean each component re-evaluates the applyFilters itself, rather than doing it just once for the shared definition. This is what I meant by "requiring the refactor" in #13231 (comment), since if we handled the debounce outside the components, it would only call applyFilters once, after the delay.

gziolo · 2019-01-08T07:22:51Z

packages/components/src/higher-order/with-filters/index.js

-					this.Component = applyFilters( hookName, OriginalComponent );
-					this.forceUpdate();
-				}, ANIMATION_FRAME_PERIOD );
+				this.throttledForceUpdate = debounce(


Yes, this sounds like a good idea.

gziolo · 2019-01-08T07:50:06Z

packages/components/src/higher-order/with-filters/index.js

+		 *
+		 * @type {Component}
+		 */
+		let FilteredComponent = applyFilters( hookName, OriginalComponent );


I suspect this line might be the reason for failing e2e tests.

Before, we would call it in the constructor, which is much later in the flow - as it is after the editor starts to render. In this case, it happens just after HOCs are applied - which happens just after the component is exported from the JS module.

I suspect this line might be the reason for failing e2e tests.

Before, we would call it in the constructor, which is much later in the flow - as it is after the editor starts to render. In this case, it happens just after HOCs are applied - which happens just after the component is exported from the JS module.

That makes sense. Maybe then we can have this be returned from some getter function which computes the value once when it's called.

Like Lodash's once:

https://lodash.com/docs/4.17.11#once

aduth · 2019-01-08T20:40:58Z

The tests appear to be passing now with the latest revisions, which include (a) not computing the filtered component definition until the first instance of the filtered component is constructed and (b) handling the throttling outside the component class so that the application of filters applies only once, and only after the debounce delay.

gziolo · 2019-01-14T12:03:18Z

I tested with the following snippet:

wp.hooks.addFilter( 'blocks.BlockEdit', 'my/filters/1', () => () => wp.element.createElement( 'h1', {}, 'My block 1' ) );

and

wp.hooks.removeFilter( 'editor.BlockEdit', 'my/filters/1' )

gziolo

This looks great and tests well. Awesome work 🚢

Ensures that FilteredComponent definition is only computed once per animation frame debounce

adamsilverstein · 2019-01-23T23:13:57Z

Excellent!

) * Components: Use single hook delegator for withFilters * Component: Share component definition for withFilters instances * Components: Use slash for withFilters subgrouping * Components: Assign FilteredComponent only once renderer constructed * Component: Handle withFilters forced update through delegator Ensures that FilteredComponent definition is only computed once per animation frame debounce

aduth added [Feature] UI Components Impacts or related to the UI component system [Type] Performance Related to performance efforts labels Jan 7, 2019

aduth requested a review from gziolo January 7, 2019 21:45

aduth commented Jan 7, 2019

View reviewed changes

gziolo requested changes Jan 8, 2019

View reviewed changes

gziolo reviewed Jan 8, 2019

View reviewed changes

gziolo approved these changes Jan 14, 2019

View reviewed changes

aduth added 5 commits January 15, 2019 15:08

Components: Use single hook delegator for withFilters

6090d1f

Component: Share component definition for withFilters instances

678ade4

Components: Use slash for withFilters subgrouping

fb7fbe2

Components: Assign FilteredComponent only once renderer constructed

81a81ad

Component: Handle withFilters forced update through delegator

274f516

Ensures that FilteredComponent definition is only computed once per animation frame debounce

aduth force-pushed the add/hooks-changed branch from 7fb880f to 274f516 Compare January 15, 2019 15:08

aduth merged commit ec1176d into master Jan 15, 2019

aduth deleted the add/hooks-changed branch January 15, 2019 15:21

youknowriad added this to the 4.9 (Gutenberg) milestone Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Optimize withFilters, avoiding per-instance bindings #13231

Components: Optimize withFilters, avoiding per-instance bindings #13231

aduth commented Jan 7, 2019

aduth Jan 7, 2019

gziolo Jan 8, 2019

gziolo left a comment

gziolo Jan 8, 2019

gziolo Jan 8, 2019 •

edited

aduth Jan 8, 2019

gziolo Jan 8, 2019

aduth Jan 8, 2019

gziolo Jan 8, 2019

aduth Jan 8, 2019

gziolo Jan 8, 2019

gziolo Jan 8, 2019

aduth Jan 8, 2019

aduth commented Jan 8, 2019

gziolo commented Jan 14, 2019

gziolo left a comment

adamsilverstein commented Jan 23, 2019

Components: Optimize withFilters, avoiding per-instance bindings #13231

Components: Optimize withFilters, avoiding per-instance bindings #13231

Conversation

aduth commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jan 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jan 8, 2019

gziolo commented Jan 14, 2019

gziolo left a comment

Choose a reason for hiding this comment

adamsilverstein commented Jan 23, 2019

gziolo Jan 8, 2019 •

edited