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

Improve FCM result processing performance #103

Merged
merged 1 commit into from
Dec 1, 2018

Conversation

treble-snake
Copy link
Contributor

Using an immutable way of composing result array lead to
a preformance issue on large numbers of tokens.

On 1kk tokens, using .push() instead of [...result, ...addend]
decreased forEach execution time from ~20000ms to ~40ms
(on the same computer).

Using an immutable way of composing result array lead to
a preformance issue on large numbers of tokens.

On 1kk tokens, using `.push()` instead of `[...result, ...addend]`
decreased `forEach` execution time from ~20000ms to ~40ms
(on the same computer).
@treble-snake
Copy link
Contributor Author

If you strictly prefer the immutable way, I could try using something like Immutable.js for this one.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a18c1c8 on treble-snake:improve-fcm-performance into 8948570 on appfeel:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a18c1c8 on treble-snake:improve-fcm-performance into 8948570 on appfeel:master.

@andys8
Copy link

andys8 commented Nov 30, 2018

Good catch. The problem is creating new arrays over and over and reassigning them. The current solution is not "strictly immutable" in terms of the result which will be reassigned multiple times.

Since there the result array is created in this function and push is used some lines above, the solution is fine.

I was interested in the performance implications and tried to compare push, spread, map + flatten, and flatMap. R is https://ramdajs.com.

Comparison

https://goo.gl/kbvo92

const n = 2000;
const m = 10

const testdata = () => R.repeat({a: R.repeat({b:Math.random()}, m)}, n)

for(let i = 0; i<10; i++) {
  
console.log("Round ", i+1);
  
console.time("forEach.push")
const message1 = [];
testdata().forEach((result) => {
  message1.push(...result.a);
});
console.timeEnd("forEach.push")

console.time("forEach.spread")
let message2 = [];
testdata().forEach((result) => {
  message2 = [...message2, ...result.a];
});
console.timeEnd("forEach.spread")

console.time("flatMap")
const message3 = R.chain(_ => _.a, testdata());
console.timeEnd("flatMap")

console.time("map.flatten")
const message4 = R.flatten(R.map(_ => _.a, testdata()));
console.timeEnd("map.flatten")

}

Results

Round  1
forEach.push: 5.030029296875ms
forEach.spread: 77.1318359375ms
flatMap: 4.0732421875ms
map.flatten: 8.080810546875ms
Round  2
forEach.push: 0.419921875ms
forEach.spread: 65.0810546875ms
flatMap: 0.821044921875ms
map.flatten: 0.816162109375ms
Round  3
forEach.push: 0.353759765625ms
forEach.spread: 73.110107421875ms
flatMap: 0.333740234375ms
map.flatten: 0.885009765625ms
Round  4
forEach.push: 0.607177734375ms
forEach.spread: 73.119873046875ms
flatMap: 0.250244140625ms
map.flatten: 0.682861328125ms
Round  5
forEach.push: 0.32177734375ms
forEach.spread: 73.404052734375ms
flatMap: 0.384033203125ms
map.flatten: 0.651123046875ms
Round  6
forEach.push: 0.552001953125ms
forEach.spread: 76.4208984375ms
flatMap: 0.2509765625ms
map.flatten: 0.6357421875ms
Round  7
forEach.push: 0.241943359375ms
forEach.spread: 73.719970703125ms
flatMap: 0.22216796875ms
map.flatten: 0.637939453125ms
Round  8
forEach.push: 0.24609375ms
forEach.spread: 72.7880859375ms
flatMap: 0.22021484375ms
map.flatten: 0.662841796875ms
Round  9
forEach.push: 0.23681640625ms
forEach.spread: 73.653076171875ms
flatMap: 0.3330078125ms
map.flatten: 1.162109375ms
Round  10
forEach.push: 0.47705078125ms
forEach.spread: 77.349853515625ms
flatMap: 0.227294921875ms
map.flatten: 0.679931640625ms

Summary

Benchmarking it in the browser is not very consistent. Sometimes is forEach.push faster in the first execution, sometimes flatMap. Overall it is clear, the current solution forEach.spread is slow.

@alex-friedl alex-friedl merged commit 8038407 into appfeel:master Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants