-
Notifications
You must be signed in to change notification settings - Fork 80
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
Idea: using for loop #59
Conversation
/cc @theKashey |
): boolean => | ||
newArgs.length === lastArgs.length && | ||
newArgs.every( | ||
(newArg: mixed, index: number): boolean => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer creating a new function on every call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a performance concern for V8, every
could be even faster!
You need a benchmark to compare with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
@@ -1,18 +1,19 @@ | |||
// @flow | |||
export type EqualityFn = (newArgs: mixed[], lastArgs: mixed[]) => boolean; | |||
|
|||
const shallowEqual = (newValue: mixed, oldValue: mixed): boolean => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function call not required: just doing it right in the for loop
src/index.js
Outdated
return false; | ||
} | ||
// Using a for loop rather than array.every for max speed | ||
for (let i = 0; i < newArgs.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, dont cache length - https://mrale.ph/blog/2014/12/24/array-length-caching.html
It looks like using the for loop is considerably faster. Given the attached benchmark, which tests:
...we get the following on Node 11.12 on a 2016 15" MacBook Pro:
Which suggests the for loop is somewhere around 25% or 30% faster all around. Here's what I ran: const Benchmark = require('benchmark');
const suite = new Benchmark.Suite();
function shallowEvery(a, b) {
if (a.length !== b.length) {
return false;
}
return a.every((e, i) => b[i] === e);
}
function shallowFor(a, b) {
if (a.length !== b.length) {
return false;
}
for (let i = 0; i < a.length; i++) {
if (a[i] !== b[i]) {
return false;
}
}
return true;
}
let a = {};
let b = {};
let listA = [a, b, {}, {}];
let listB = [a, b, {}, {}];
suite.add('shallowEvery with identical lists', () => {
shallowEvery(listA, listA);
});
suite.add('shallowFor with identical lists', () => {
shallowFor(listA, listA);
});
suite.add('shallowEvery with half-identical lists', () => {
shallowEvery(listA, listB);
});
suite.add('shallowFor with half-identical lists', () => {
shallowFor(listA, listB);
});
suite.on('cycle', e => console.log(String(e.target)));
suite.run({ async: true }); |
Note that in the following case of longer arrays, the gap closes to about a 10% improvement with the c-style for loop.
Gives:
We should keep in mind this is all on V8, and a newer version of Node at that. Might be worth testing in other environments. |
The difference seems significant enough to warrant a move |
Just ran the benchmark through browserify and tried other environments: Safari 12.1:
Firefox 66:
Definitely seems worthwhile 😄 |
On my local (node 8.11.3) shallowEvery with identical lists x 7,977,614 ops/sec ±1.30% (84 runs sampled)
shallowFor with identical lists x 117,648,438 ops/sec ±0.59% (88 runs sampled)
shallowEvery with half-identical lists x 8,780,743 ops/sec ±1.54% (82 runs sampled)
shallowFor with half-identical lists x 124,660,454 ops/sec ±0.72% (88 runs sampled) ~16x faster 😲 |
Look like |
node 10.9.0
|
Moving to for loop for max speed. Need to run some more performance tests