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

nested reducer composition - performance question #634

Closed
ms88privat opened this issue Aug 27, 2015 · 19 comments
Closed

nested reducer composition - performance question #634

ms88privat opened this issue Aug 27, 2015 · 19 comments
Labels

Comments

@ms88privat
Copy link

Hello,

my App is getting very large and so are my reducers. I split them into multiple files and structured them by nesting.

It looks something like this:

// I MUST define this initalState, otherwise 'state.entities.user'...  would not be defined
const initalState = {
  entities: {},
  collections: {},
  ....
};

function reducer(state = initalState, action) {
  return {
    // every entity is managed by its own reducer (the difference to the real world example)
    entities: {
      users: entitesUserReducer(state.entities.user, action),
      images: entitiesImagesReducer(state.entities.images, action),
      ....
    },
    // subsets of the entities (arrays of id's)
    collections: {
      usersTop: collectionsUsersTopReducer(state.collections.usersTop, action),
      ....,
    temp: {
      selectedUser: tempSelectedUserReducer(state.temp.selectedUser, action),
      ....
    },
    ...
  }
}

...
let store = createStoreWithMiddleware(reducer);
...

It is working fine, but my question is now, what happens when my "entities.users" change? Are the components which are listening for "entities.images" re-rendered too? (using connect@react-redux here)

So is the performance the same compared to not nesting (with the same reducer count) and besides the deeper object comparison which is negligible?

I'm not 100% sure how redux work here, I have to analyse the source code yet. But i wanted to share this example how to deep nest the reducers anyway.

Thanks,

@srph
Copy link

srph commented Aug 27, 2015

They are re-rendered, but I don't see any performance hit if you're using React (since they're _diff_ed). I'm not sure how and why you'd have multiple @connect though (I'd probably have a separate @connect for the navigation)

@danmaz74
Copy link
Contributor

To keep the performance... performant, it's up to you to use shouldComponentUpdate wisely in your components tree. The easiest way to do so is to use Immutable.js or something similar for your state - this way, to know if a particular sub-tree of the state has changed, shouldComponentUpdate can simply compare the reference. In other words, with immutability, you can use the PureRenderMixin freely in your components.

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2015

It is working fine, but my question is now, what happens when my "entities.users" change? Are the components which are listening for "entities.images" re-rendered too? (using connect@react-redux here)

No. If your images reducer looks like

function images(state, action) {
  switch (action.type) {
  case DO_SOMETHING_WITH_IMAGES:
    return transformSomehow(state);
  default:
    return state; // <--------- note! returning same reference for any unknown action
  }
}

and if you're listening to state.entities.images, React Redux will compare state.entities.images with previousState.entities.images. Because a users-related action did not change the reference to images, even though state.entities changed, the view listening to state.entities.images does not re-render.

Of course, if the view is listening to state.entities directly, it will re-render, but that's the point: you need to subscribe to only the parts of the state your view cares about. In addition, you can use reselect to compute derived data.

I'm not 100% sure how redux work here, I have to analyse the source code yet. But i wanted to share this example how to deep nest the reducers anyway.

It's not really related to Redux. Redux store will call every listener. However React Redux connect() is rather picky about when to update.

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2015

They are re-rendered

This is not correct, I think. If they are re-rendered, it means your nested state reducer returns a new object even when the state should not have changed. You need to write your reducers in such a way that they ignore unrelated actions.

To keep the performance... performant, it's up to you to use shouldComponentUpdate wisely in your components tree.

Note that connect() already has a very aggressive shouldComponentUpdate. In most apps it should be enough, although, if you have lots of data, views lots of updates, using Immutable helps even more.

@ms88privat
Copy link
Author

React Redux will compare state.entities.images with previousState.entities.images

Thanks a'lot, thats what I'm looking for. So i'm good to go, I use reselect and return same ref if no change happened.

@danmaz74
Copy link
Contributor

@gaearon then maybe I didn't understand the docs well, or there is something else I'm missing.

Then, we wrap the components we want to connect to Redux with the connect() function from react-redux. Try to only do this for a top-level component, or route handlers.

This is what we're doing; currently in the app we're refactoring with Redux we have only one top-level component which is connected to the store, and a big enough tree of "dumb" components that descend from that root. If you connect only one top level component, which gets the whole state, won't it re-render every time there is any change in the state? That's why we used Immutable and PureRenderMixin in all the subcomponents; our idea is that the top level component will update, but then all the subcomponents that aren't affected by the change will actually not render.

Doesn't the connect's aggressive shouldComponentUpdate only help if you have lots of small trees of components, each connected to the store, instead of one big tree? Or a few big trees?

@ms88privat
Copy link
Author

I think your question is

if this ('connect' only at the top level)

mapStateToProps(state) {
  return {
    users: state.entities.users,
    images: state.entities.images,
  };
}
@connect(...)
class TopLevelComponent extends ...

  render() {
    <UserComponent users={this.props.users} />
    <ImageComponent images={this.props.images} />
  }

is equal in performance compared to ('connect' in every sub component)

class TopLevelComponent extends ...

  render() {
    <UserComponent />
    <ImageComponent  />
  }

and

mapStateToProps(state) {
  return {
    users: state.entities.users,
  };
}
@connect(...)
class UserComponent extends ...

  render() {
    // do something with users
  }

and

mapStateToProps(state) {
  return {
    images: state.entities.images,
  };
}
@connect(...)
class ImageComponent extends ...

  render() {
    // do something with images
  }

I think it should be the same?

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2015

@danmaz74

If you connect only one top level component, which gets the whole state, won't it re-render every time there is any change in the state?

Yes, you're right. Connecting only top-level component is by default slower than connecting components in the middle.

The point is: use single connect() at the top until it's either

  • slow, or
  • too tedious to pass the props down.

When either of these things happen, move connect() down to solve these problems.
See also #419 for a discussion.

The problem is with the docs, not with the implementation. We should explain more clearly about the tradeoffs of different strategies. Both “just one connect” and “connect every single component” aren't very good tradeoffs in my opinion, you should strive for the middle.

@danmaz74
Copy link
Contributor

@gaearon I get the point; for ourselves, we were thinking to use multiple "connect" at a later stage, when we'll have one or more unrelated component trees.

Anyway, shouldn't using Immutable + PureRenderMixin improve the performance a lot in a situation like that? Most of the performance problems should probably come from the "inner loop" leaf components, for example when there is a big list of components inside another component, and I would expect shouldComponentUpdate() very quickly returning false for those to alleviate the problem a lot (but I never tested it yet).

@gaearon
Copy link
Contributor

gaearon commented Aug 27, 2015

Anyway, shouldn't using Immutable + PureRenderMixin improve the performance a lot in a situation like that?

That's right. It's just not something we can enforce, and not really different from optimizing non-Redux React app, so I don't talk about it much—it's in React's documentation.

@danmaz74
Copy link
Contributor

@ms88privat of course the performance will be worse with only one connect, but, as @Gaeron says, there is also a tradeoff in using "many connects".

Moreover, I can think about situations where I think that using "many connects" is actually impossible - ie when you have a tree of components that is deep and structurally dependent on the status/props of the root.

@danmaz74
Copy link
Contributor

@gaearon ok thanks that's what I wanted to be sure about.

@joaovpmamede
Copy link

@gaearon I'm have an issue where I have a combineReducers like:

export default combineReducers({
  form: formReducer,
  api:  apiReducer
});

state.form is being changed because of lots of actions being dispatch, such as: onFocus, onBlur, etc...
problem is that on each of my connects that use state.api:

export default connect(({ api }) => ({
  foo: api.foo
}))(Component);

I'm getting all of their children components re-rendered.

From what I understood:

and if you're listening to state.entities.images, React Redux will compare state.entities.images with previousState.entities.images. Because a users-related action did not change the reference to images, even though state.entities changed, the view listening to state.entities.images does not re-render.

it shouldn't re-rendered.
Although I know that the global state object coming from combineReducers changed.

@gaearon
Copy link
Contributor

gaearon commented Dec 29, 2015

@joaovpmamede

This shouldn't be happening. Are you sure state.api.foo doesn't get changed on arbitrary actions? Can you show its source?

@joaovpmamede
Copy link

@gaearon

Sure. Basically every action dispatched from redux-form is changing the state.form, but state.api is being returned "unchanged".

export function apiReducer(state = {}, action) {
  if (!action.type.startsWith('API_')) return state;
  ...

  return state;  // not needed but it's there
}

I've also debugged this and I can confirm it returns the current state.


Edit:

Just to check if I'm correctly understanding this...
On the shopping cart example, if you change the products reducer to something like:

function products(state, action) {
  return state;
}

then if you click Add to Cart button, the code on the ProductsContainer:

function mapStateToProps(state) {
  return {
    products: getVisibleProducts(state.products)
  }
}

export default connect(
  mapStateToProps,
  { addToCart }
)(ProductsContainer)

shouldn't change the state and won't re-render, correct?

@gaearon
Copy link
Contributor

gaearon commented Dec 30, 2015

It will cause a re-render because the getVisibleProducts selector always creates a new value. If you want to optimize this, you should use a library like reselect. Please see http://redux.js.org/docs/recipes/ComputingDerivedData.html.

@joaovpmamede
Copy link

@gaearon right, that in the shopping cart example but on my case it doesn't create a new value, just returns state.
Not sure what's happening and I had to add some conditions on shouldComponentUpdate.

@gaearon
Copy link
Contributor

gaearon commented Dec 30, 2015

I can't help without a project reproducing the issue.

@joaovpmamede
Copy link

I understand and sorry about that.
Anyway just made more tests with the examples provided and it seems everything is working accordingly.

Thanks.

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

No branches or pull requests

5 participants