Skip to content

Commit

Permalink
Use a for loop instead of forEach() in dispatch()
Browse files Browse the repository at this point in the history 
We don't care about holey array semantics because we manage the array ourselves
  • Loading branch information
@gaearon
gaearon committed Jan 31, 2016
1 parent 763fa83 commit 5b58608
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/createStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,11 @@ export default function createStore(reducer, initialState, enhancer) {
isDispatching = false
}

listeners.slice().forEach(listener => listener())
var currentListeners = listeners.slice()
for (var i = 0; i < currentListeners.length; i++) {
currentListeners[i]()
}

return action
}

Expand Down

15 comments on commit 5b58608

@gajus
Copy link
Contributor

@gajus gajus commented on 5b58608 Jan 31, 2016

Choose a reason for hiding this comment

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

What makes this a performance optimisation?

@ColadaFF
Copy link

Choose a reason for hiding this comment

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

http://jsfiddle.net/ssSt5/2/ @gajus

@bvaughn
Copy link

Choose a reason for hiding this comment

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

Wouldn't it be slightly more efficient to just store the initial length of the listeners array and only iterate up to that index (rather than copying the array)? I assume you're copying it in case a listener results in a new subscriber being added to the array- but since subscribers are added onto the end of the array... it seems like storing the initial length would be sufficient.

(I think it's actually a tiny bit more performant to store length in a var before iterating anyway.)

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

This is an optimization because forEach() has more complicated logic per spec to deal with sparse arrays. Also it's better to not allocate a function when we can easily avoid that.

No, we can't just limit ourselves to the last index because subscriptions can also be removed. However we do have an optimization to avoid cloning the array on every dispatch: see the next few commits.

@bvaughn
Copy link

@bvaughn bvaughn commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

Yeah, sorry Dan. I looked at this commit before noticing the follow-up commit. :)

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

As for your last point, I believe modern engines optimize array.length access in the loop anyway and hoisting that call doesn't make it faster.

@tappleby
Copy link
Contributor

Choose a reason for hiding this comment

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

However we do have an optimization to avoid cloning the array on every dispatch: see the next few commits.

@gaearon if you ping me once the optimizations are complete I can migrate them to redux-batched-subscribe.

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

@tappleby So far I don't think there's anything I can squeeze out of this

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

@tappleby You'll want to look at three recently added tests in createStore.spec.js as there's some behavior that is hard to get right when optimizing

@gajus
Copy link
Contributor

@gajus gajus commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

As for your last point, I believe modern engines optimize array.length access in the loop anyway and hoisting that call doesn't make it faster.

Not sure about that.

var index,
    test;

test = {};

Object.defineProperty(test, 'count', {
    get () {
        console.log('A');

        return 100;
    }
})

for (index = 0; index < test.count; index++) {
    console.log('B');
}

https://jsfiddle.net/qw1y8g3y/

test.count is assessed on each iteration, the same way that test.length would.

That said, I could not confirm this using JSPerf, http://jsperf.com/quick-for-vs-foreach. Maybe the implication of testing length is negligible.

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 1, 2016

Choose a reason for hiding this comment

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

V8 (and other engines too) are smarter than we give them credit for.
http://mrale.ph/blog/2014/12/24/array-length-caching.html

@sompylasar
Copy link

Choose a reason for hiding this comment

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

And more food for thought: http://mrale.ph/blog/2012/12/15/microbenchmarks-fairy-tale.html

@sompylasar
Copy link

Choose a reason for hiding this comment

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

@gaearon GMTA

@gajus
Copy link
Contributor

@gajus gajus commented on 5b58608 Feb 2, 2016

Choose a reason for hiding this comment

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

This is an optimization because forEach() has more complicated logic per spec to deal with sparse arrays. Also it's better to not allocate a function when we can easily avoid that.

This recent knowledge saved me countless of hours when fixing react-css-modules performance issue (https://github.com/gajus/react-css-modules/issues/89). Thank you. 👍

@gaearon
Copy link
Contributor Author

@gaearon gaearon commented on 5b58608 Feb 2, 2016

Choose a reason for hiding this comment

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

Glad to help!

Please sign in to comment.