-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Immutable data + bad performance #1262
Comments
@elado, have you tried ImmutableJS by any chance? I created an example method that fits your jsbin: var users = _.times(1000).map(i => ({ type: 'user', id: i, name: `user ${i}` }));
var immutableUsers = Immutable.fromJS(users);
function immutableMerge() {
return immutableUsers.reduce((state, u, key, iter) => {
return state.mergeDeep({ [u.get('type')]: { [u.get('id')]: u } });
}, Immutable.fromJS({ user: {} }))
} { lodashMerge: { state: { user: [Object] }, duration: 2281 },
immutableMerge: { state: { user: [Object] }, duration: 62 } } |
@babsonmatt hmm this is the output I get:
so |
I'm a bit confused as to what the test is doing. Why does it copy every single entity? |
@babsonmatt Thanks! I'll explore this today @pke yes, obviously mutable is the fastest but it won't work with how redux expects the data, which is immutable
Maybe it's ok to assume that the app doesn't care about edit 2 updated version of the jsbin: http://jsbin.com/mosiri/2/edit?js,console |
So the server response is like this: [{
type: 'user',
id: 0,
name: 'user 0'
}, {
type: 'user',
id: 1,
name: 'user 1'
}] and the state structure you want to get is like this: {
user: {
0: {
type: 'user',
id: 0,
name: 'user 0'
},
1: {
type: 'user',
id: 1,
name: 'user 1'
}
}
} Is this correct? |
@gaearon It could be an array like this or a single entity that I run an action to merge inside |
As for |
|
My data comes in a stream of entities that come in different times, each entity or a group of entities needs to be merged to the state. To prevent UI re-render on each action, I debounce it. Explained here: reduxjs/react-redux#263 I'll probably end up using Immutable.js as it seems the fastest (interesting to see how it's solved inside). |
This version is faster than the function somehow() {
// These are supposed to be derived from state and action arguments
let prevState = { user: {} };
let fetchedEntities = users;
// At first, shallowly copy the old state
let newState = { ...prevState };
let hasChanged = {};
fetchedEntities.forEach(e => {
let { type, id } = e;
let newEntities = newState[type];
// Unknown entity type: create an empty object
if (!newEntities) {
newEntities = newState[type] = {};
}
// If fetched entity is equal, avoid changing its reference
if (_.isEqual(e, newEntities[id])) {
return;
}
// Produce a new object (once!) if we're handling an entity of that type.
// It's only created here so the function stays pure despite the internal mutation.
if (!hasChanged[type]) {
newEntities = newState[type] = { ...newEntities };
hasChanged[type] = true;
}
// Reassign the entity (you can also merge with old version if you'd like)
newEntities[id] = e;
});
return newState;
} It also avoids changing the references for old dictionaries and individual items when we know they haven't changed. |
To clarify this. For example this is fine: function reducer(state, action) {
let arr = [state.something];
arr.push(action.something);
return arr;
} I am mutating an array I just created so this mutation is completely unobservable from outside and it doesn't make the function any less pure—I can call it many times and get the same results. However this is a bad mutation: function reducer(state, action) {
state.arr.push(action.something);
return state;
} In this case the mutation is observable and it will change the inputs, which is what we don't allow. If a tree falls in the forest and nobody hears it, did it really fall? |
function lodashMerge2() {
let prevState = { user: {} };
let fetchedEntities = users;
let stateToMerge = {};
fetchedEntities.forEach(e => {
let { type, id } = e;
stateToMerge[type] = stateToMerge[type] || {};
stateToMerge[type][id] = e;
});
return _.merge({}, prevState, stateToMerge);
}
Scratch that, I don't think it's a good version because |
At
real-world
'sentities
reducer implementation, https://github.com/rackt/redux/blob/master/examples%2Freal-world%2Freducers%2Findex.js#L10This approach seems to be super slow with bigger data sets (say, 1000 entities).
lodash.merge
has its own performance problem, as 1000 entities take 2.5s, but spread copy{...state, ...}
is still slow (~120ms)Here is a comparisons of merge/spread/partial copy/mutable: http://jsbin.com/miroto/3/edit?js,console
Obviously, Redux lives on immutable data, so it needs to stay immutable. How do you deal with this performance hit when dealing with many entities?
The text was updated successfully, but these errors were encountered: