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

Data: Optimize withSelect to avoid generating merge props on equal props #7699

Merged
merged 2 commits into from
Jul 4, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 3, 2018

Closes #7680
Closes #6466
Closes #7498

This pull request seeks to implement the data module's withSelect implementation to avoid a case where mapSelectToProps would be called wastefully when neither the state had triggered a change nor had incoming props changed (as a product of shallow equality comparison).

This had been effected in previous implementations leveraging componentWillReceiveProps, but was lost in the upgrade to using static getDerivedStateFromProps:

componentWillReceiveProps( nextProps ) {
if ( ! isShallowEqual( nextProps, this.props ) ) {

It required some refactoring to account for state changes made by the artificial trigger from the component's subscription, to handle generating new merge props on either a state change or incoming props change, while ensuring that the component only re-renders when either props or merge props are shallowly unequal, also ensuring that isShallowEqual is called at most one time per props sets during this render pass.

In future iterations, it may be worth exploring to see whether props and fromStoreUpdate can be omitted from state. I'd considered assigning instance variables in shouldComponentUpdate and the subscription callback, where in the latter we'd call forceUpdate to skip shouldComponentUpdate. The main downside to this is that shouldComponentUpdate should ideally not have side effects such as assigning instance variables.

Testing instructions:

Ensure new unit tests pass:

npm run test-unit

Observe lag or CPU usage differences from this branch in contrast with master.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Performance Related to performance efforts labels Jul 3, 2018
@aduth aduth added this to the 3.2 milestone Jul 3, 2018
@aduth
Copy link
Member Author

aduth commented Jul 3, 2018

While this is likely a valid optimization, in some testing this is still not even close to the same level of performance. Typing in the demo post, for example, is unbearably slow, even after these changes, when contrasted with the variation of withSelect from 95ef796.

@aduth
Copy link
Member Author

aduth commented Jul 3, 2018

In future iterations, it may be worth exploring to see whether props and fromStoreUpdate can be omitted from state. I'd considered assigning instance variables in shouldComponentUpdate and the subscription callback, where in the latter we'd call forceUpdate to skip shouldComponentUpdate. The main downside to this is that shouldComponentUpdate should ideally not have side effects such as assigning instance variables.

This approach turned out to be much more viable, and made a night-and-day difference to the usability of the demo post. I'm still not entirely clear what was problematic about the previous implementation: It may be that getDerivedStateFromProps is simply not performant, or due to the additional memory overhead of tracking props and fromStoreChange in state.

@aduth aduth added the [Priority] High Used to indicate top priority items that need quick attention label Jul 4, 2018
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

The last iteration of this PR makes a huge difference for me when editing the demo post.

I'd have a second set of eyes on this, but it looks like solid work. Yes, concessions are made, but this is quite the hot path.

@@ -529,6 +529,64 @@ describe( 'withSelect', () => {
expect( OriginalComponent ).toHaveBeenCalledTimes( 2 );
} );

it( 'should not run selection if props have not changed', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This one fails in master because mapSelectToProps gets called twice. This is a very good indicator that this PR fixes the issue.

*/
function getNextMergeProps( props ) {
return (
mapStateToProps( select, props ) ||
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: we should rename mapStateToProps to mapSelectToProps.

@gziolo
Copy link
Member

gziolo commented Jul 4, 2018

Yes, I can confirm that it works much better. I was able to confirm that we don't notice double re-renders anymore in both testing and when comparing with the master branch.
Although proposed code uses some not-recommended solution as noted in the inline comment, it is much easier to follow and in my opinion works exactly as we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants