-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Components: Optimize withFilters, avoiding per-instance bindings #13231
Conversation
this.Component = applyFilters( hookName, OriginalComponent ); | ||
this.forceUpdate(); | ||
}, ANIMATION_FRAME_PERIOD ); | ||
this.throttledForceUpdate = debounce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this sounds like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit more about it, it is probably correct. I mean, that when mounting it always increments the counter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
this.Component = applyFilters( hookName, OriginalComponent ); | ||
this.namespace = uniqueId( 'core/with-filters/component-' ); | ||
this.throttledForceUpdate = debounce( () => { | ||
this.Component = applyFilters( hookName, OriginalComponent ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 thethrottledForceUpdate
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) ?
} | ||
|
||
// Recreate the filtered component. | ||
FilteredComponent = applyFilters( hookName, OriginalComponent ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
this.Component = applyFilters( hookName, OriginalComponent ); | ||
this.forceUpdate(); | ||
}, ANIMATION_FRAME_PERIOD ); | ||
this.throttledForceUpdate = debounce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this sounds like a good idea.
* | ||
* @type {Component} | ||
*/ | ||
let FilteredComponent = applyFilters( hookName, OriginalComponent ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
:
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. |
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' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great and tests well. Awesome work 🚢
Ensures that FilteredComponent definition is only computed once per animation frame debounce
7fb880f
to
274f516
Compare
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
) * 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
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 inaddHook
,runHooks
, andFilteredComponent#constructor
. These changes bring this down to roughly 170ms, withrunHooks
being the primary contributor (160ms), as it's also used extensively elsewhere in blocks parsing (i.e.withFilters
contribution approaches negligible).Implementation notes:
The specific optimizations include:
addFilter
andremoveFilter
per hook name. Previously, a handler was added for each instance of a 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).https://reactjs.org/docs/react-component.html#constructor
Testing instructions:
Repeat testing instructions from #4428, substituting the hook name
blocks.BlockEdit
with the since-renamededitor.BlockEdit
. In other words, insert this code in your console: