-
Notifications
You must be signed in to change notification settings - Fork 380
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
Fix histogram scrape performance #219
Conversation
When profiling histogram scraping I discovered that the major bottleneck is creating a clone of the accumulator in reduce. We should not actually need to skip mutation.
lib/histogram.js
Outdated
@@ -288,7 +288,7 @@ function extractBucketValuesForExport(histogram) { | |||
|
|||
function addSumAndCountForExport(histogram) { | |||
return (acc, d) => { | |||
acc = acc.concat(d.buckets); | |||
acc.push.apply(acc, d.buckets); |
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.
why not just acc.push(d.buckets);
?
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.
oh, it's an array. makes sense :)
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 can be acc.push(...d.buckets);
, we have a minimum node version of 6
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.
yeah, I didn't look up minimum node version, I had it that way initially but then decided to start off with more support. I'll update to the ES6 syntax.
Thanks for the PR! Mind updating the changelog as well? |
Done |
Thanks for the merge @SimenB! I did not package bump because I assume that you do that as part of your release process. Thanks for the great package, and for such a quick response! |
It's released as 11.1.2 now 🙂 |
❤️ thanks!!! |
So nice!! Thank you! ❤️ |
When profiling histogram scraping I discovered that the major bottleneck is creating a clone of the accumulator in reduce. We should not actually need to skip mutation.
You can see the benchmark of
concat
vspush
here: https://codeburst.io/jsnoob-push-vs-concat-basics-and-performance-comparison-7a4b55242fa9This should address #216
I used this test script to profile.
Which resulted in before this patch of
7.021s
and after of0.501s