Wasteful operations when mapStateToProps has 2 arguments, store triggers change event but props and state unchanged

[ghost]

As title suggests, when mapStateToProps is using both state and ownProps to select chunk of redux state, when store has some state updated, but the particular chunk of state which is returned by mapStateToProps has not been changed, connect produces wasteful render attempt. This is specifically bad in places when there is thousands of components wrapped with connect, or there is just couple of such components, store is updated quite frequently, but state of wrapped components is not changed. Issue is not reproduced when mapStateToProps is not using ownProps.

Performance degradation is seen in the task manager and in JS profiler, and becomes a visually noticeable problem on documents with complex DOM in Edge and Firefox. Measurements from React.addons.perf.printWasted show that on 10 seconds intervals the wasteful operations can take more than 1 second, the wasteful operations are happening inside Connect(Component).

Looking into profiling using Chrome profiler, a lot of time is spent inside ReactComponent.setState, looking into source code of connect.js, the problem is apparently in the handleChange.js, it does an unconditional this.setState when this.doStatePropsDependOnOwnProps is true. Perhaps an option to customize this behavior, e.g. by introducing support for custom comparer would be really nice.

Small sample project with reproduction has been set in https://github.com/vlan-saxo/react-redux-bug, use npm install, npm start and go to http://localhost:8080/webpack-dev-server/

[larpo]

I ran into the same issue while going through react perftools results. There probably is a valid reason for the doStatePropsDependOnOwnProps check, but I think the docs should be more clear about this limitation.

Anyway, the workaround in my case was to use a custom mergeProps function, eg. changing from

connect(
    ({items}, {selected}) => ({item: items[selected]})
)(Component)

to

connect(
    ({items}) => ({items}),
    null,
    ({items}, _ , parentProps) => ({...parentProps, item: items[parentProps.selected] })
)(Component)

Looks ugly, but the perf issue is gone.

[jimbolla]

I think this will be addressed by #407. I'm rewriting the Connect component to only fire setState() if the final props have changed. I'm seeing HUGE performance gains.

[markerikson]

@jimbolla : yeah, this would be another good before/after comparison project.

[gaearon]

This is specifically bad in places when there is thousands of components wrapped with connect

Note that this is usually not a great idea by itself; something like https://bvaughn.github.io/react-virtualized/ is much better for performance.

[markerikson]

True. That said, not every example of "lots of connected items" would fall right into the "single list" use case.

@jimbolla , it'd really be nice to actually get some hard numbers on just what the overhead is from the current connect() implementation, and how many connected components is "too much".

[jimbolla]

@markerikson My latest comment on #407 has some insight into that. I have 331 connected components running. Current implementation is getting single digit FPS, my version is "good enough for console gamers." hehehe. I don't know if 1000s will ever be feasible without some radical unforeseeable change. But certainly a few dozen at once should be doable.

[markerikson]

Cool. Now, haven't looked at the source for any of these benchmarks, but I know that the 10K list item example in the "High Performance Redux" slides (as inspired by the MobX "Redux TodoMVC" repo) goes from having 1 connected list and being basically unusable, to having 1 connected list + 10K connected list items and vastly better. So, number of connections is a factor, also presumably frequency of actions, but I'd really love to get a better idea what that curve looks like.

[jimbolla]

I guess it depends on how often it's dispatching actions. The project I'm using is doing

  setInterval(updatePair, 13)
  setInterval(updatePair, 21)
  setInterval(updatePair, 34)
  setInterval(updatePair, 55)

to simulate a stock ticker, so that would be 1000/13 + 1000/21 + 1000/24 + 1000/55 = 184.39 actions dispatched per second. But if your actions are more based on user interactions then you won't be doing anything near that frequency.

[markerikson]

That.... seems like an acceptable stress test :)

[jonkelling]

@jimbolla thanks for all the commitment to this! 👍 for the stress test metrics! You are 2 or 3 steps ahead of me, it seems, so I'm trying to do my best to keep up with the posts here :)

@markerikson Thanks for clarifying what that commented-out code does--I got that impression but wasn't sure if that was really the reason. Now, knowing that :), it was pretty easy to spot the implications (without writing more exhaustive test cases). Actually, on that note, . So...I ask this... @markerikson @jimbolla what should I do to help you guys?? haha. I am on the fence here. I envision myself slowly getting the to the point of wanting to rewrite connect like @jimbolla, but I also feel like that's too big a shift if a solution to performance can come from handling an edge case or two--perhaps a small fix first, a bigger rewrite second, with more tests in between?

A unit test can be written against the current code to expose the case that leads to the performance problems. It may be a few days until I can get back to this, but if I can sneak something useful in, I'll do my best!

[jimbolla]

@jonkelling If you can write any new tests, that would be great. If you create them in a fork, I can pull into my fork. I'm working on the "cover letter" that will go with the eventual PR for #407, hopefully done by this weekend.

[PCreations]

This issue is problematic when ownProps is just used once for the lifetime of the component. For example, when you a have a huge list such as the 10k list exemple, all items are connected and depends on their ownProps to select the correct item from store :

connect(
    (state, { id }) => ({
        item: getItem(state, id)
    )}
)(ListItem)

For any item modified, all the connected component will rerender due to this line in handleChange :

if (pure && !this.doStatePropsDependOnOwnProps) { [...] } this.hasStoreStateChanged = true;

Which literally means "if the connected component depends on ownProps, we don't even check for stateProps changes and always set the hasStoreStateChanged to true". Resulting in wasted time in all <Connect(YourComponentThatDependsOnItsOwnProps)>.

A simple workaround is to use the notion of initial props, as stated in the 10k list example to get ride of the props dependency. It's safe because these kind of props are never intended to change. mapStateToProps now looks like this :

connect(
    (_, { id }) => (state) => ({
        item: getItem(state, id)
    )}
)(ListItem)

[jimbolla]

#416 will fix this. If getItem returns the same value as previous call, Connect will skip unnecessary re-renders.

[mondaychen]

Correct me if I didn't understand this correctly.

In current connect we have this if statement:

if (pure && !this.doStatePropsDependOnOwnProps) {
// do shallow comparing to see if re-render is needed
}

This means any component depends on ownProps will always be re-rendered if any part of the state is changed. Why do we skip shallow comparing in this case?

[PCreations]

Exactly, and I don't know why either...