Optimize things a little bit #1

Merged
merged 7 commits into from Apr 13, 2016

Conversation

Projects
None yet
7 participants
@gaearon
Contributor

gaearon commented Apr 12, 2016

I updated the code to be more idiomatic Redux. I think it performs a little better in my testing.
I might be able to take another look tomorrow evening in case there’s something else I could optimize.

The basic idea here (apart from minor refactorings) is to separate independently changing data (the list of items and items themselves). This, of course, highly depends on the specific app, which is why we don’t have a proper guide for doing it, but the point is that it’s usually possible to optimize an app for specific use cases if there is a need for it.

Computing Derived Data is probably the best we have in terms of guides on how to do it, but it definitely could use some state structure refactoring advice content.

cc @ellbee for no specific reason—just in case he’s interested

@capaj

This comment has been minimized.

Show comment
Hide comment
@capaj

capaj Apr 12, 2016

This is freaking awesome. I can't wait to see the updated charts @mweststrate!

capaj commented Apr 12, 2016

This is freaking awesome. I can't wait to see the updated charts @mweststrate!

@Kerumen

This comment has been minimized.

Show comment
Hide comment
@Kerumen

Kerumen Apr 12, 2016

Just wondering why you add a shouldComponentUpdate even if the component is connected?
If I read correctly the react-redux docs, you can specifiy if a component implements a shouldComponentUpdate with the fourth argument of connect. The default is true. So by default, all components connected are pure, there is no need to write it.

No doubt you are aware of this, I didn't understand it correctly I guess.

Kerumen commented Apr 12, 2016

Just wondering why you add a shouldComponentUpdate even if the component is connected?
If I read correctly the react-redux docs, you can specifiy if a component implements a shouldComponentUpdate with the fourth argument of connect. The default is true. So by default, all components connected are pure, there is no need to write it.

No doubt you are aware of this, I didn't understand it correctly I guess.

@mweststrate

This comment has been minimized.

Show comment
Hide comment
@mweststrate

mweststrate Apr 12, 2016

Owner

@Kerumen you mean like in the original? For the first scenario (without selectors) TodoItem didn't use connect at all but just received it's todo item from the parent. So not using PureRenderMixin would have caused all TodoItem components to re-render if a todo changes, instead of only the affected ones.

Owner

mweststrate commented Apr 12, 2016

@Kerumen you mean like in the original? For the first scenario (without selectors) TodoItem didn't use connect at all but just received it's todo item from the parent. So not using PureRenderMixin would have caused all TodoItem components to re-render if a todo changes, instead of only the affected ones.

@Kerumen

This comment has been minimized.

Show comment
Hide comment
@Kerumen

Kerumen Apr 12, 2016

No I was asking for the @gaearon rewrite. For instance here.

Kerumen commented Apr 12, 2016

No I was asking for the @gaearon rewrite. For instance here.

@michaseel

This comment has been minimized.

Show comment
Hide comment
@michaseel

michaseel Apr 12, 2016

@gaearon Could you update the unit tests too? I would like to see how you test stateless functional components!

@gaearon Could you update the unit tests too? I would like to see how you test stateless functional components!

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 12, 2016

Contributor

Well, it’s not that much of an improvement but it’s definitely better. Still can’t avoid going through 10K subscribers. There may be small things to optimize in React Redux as well so I’ll look there.

I removed shouldComponentUpdate later, please see the complete change list. I added it for testing in one of the commits but decided it is unnecessary.

No, alas, I won’t be adding tests here, you should check out Redux repo (and open PRs to it) to see how to test components.

Contributor

gaearon commented Apr 12, 2016

Well, it’s not that much of an improvement but it’s definitely better. Still can’t avoid going through 10K subscribers. There may be small things to optimize in React Redux as well so I’ll look there.

I removed shouldComponentUpdate later, please see the complete change list. I added it for testing in one of the commits but decided it is unnecessary.

No, alas, I won’t be adding tests here, you should check out Redux repo (and open PRs to it) to see how to test components.

@slorber

This comment has been minimized.

Show comment
Hide comment
@slorber

slorber Apr 12, 2016

Still can’t avoid going through 10K subscribers.

I guess 10k subscribers is better than 10k items list reconciliation.

But there are potential ways to avoid calling those 10k subscribers on every item change. Didn't have time to work on something concrete yet but it can be optimized further imho. See reduxjs/react-redux#269

slorber commented Apr 12, 2016

Still can’t avoid going through 10K subscribers.

I guess 10k subscribers is better than 10k items list reconciliation.

But there are potential ways to avoid calling those 10k subscribers on every item change. Didn't have time to work on something concrete yet but it can be optimized further imho. See reduxjs/react-redux#269

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Apr 12, 2016

Contributor

Yeah, there are definitely ways to solve this even with vanilla Redux. The goal here is to avoid complicating the code beyond recognition 🙂

Contributor

gaearon commented Apr 12, 2016

Yeah, there are definitely ways to solve this even with vanilla Redux. The goal here is to avoid complicating the code beyond recognition 🙂

+import React, { PropTypes } from 'react'
+import Header from '../components/Header'
+import MainSection from '../components/MainSection'
+import * as TodoActions from '../actions'

This comment has been minimized.

@narqo

narqo Apr 12, 2016

This TodoActions import is useless here, isn't it?

@narqo

narqo Apr 12, 2016

This TodoActions import is useless here, isn't it?

This comment has been minimized.

@gaearon

gaearon Apr 12, 2016

Contributor

Yea, missed this one. Doesn't affect the perf though.

@gaearon

gaearon Apr 12, 2016

Contributor

Yea, missed this one. Doesn't affect the perf though.

@mweststrate

This comment has been minimized.

Show comment
Hide comment
@mweststrate

mweststrate Apr 12, 2016

Owner

Thanks for the contributions! Will re-run benchmarks tomorrow.

Owner

mweststrate commented Apr 12, 2016

Thanks for the contributions! Will re-run benchmarks tomorrow.

@mweststrate mweststrate merged commit 9b12801 into mweststrate:master Apr 13, 2016

@gaearon gaearon referenced this pull request in reduxjs/redux May 22, 2016

Closed

Performance issues with large collections #1751

@markerikson markerikson referenced this pull request in reduxjs/redux May 30, 2016

Open

Add an "Optimization" recipe #1783

@Yang03 Yang03 referenced this pull request in Yang03/react-isomorphic Sep 5, 2016

Open

redux performance #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment