perf: improve perf by refactoring code to be cleaner and more efficient #559
Conversation
There was a problem hiding this comment.
Benchmarks so far:
✓ registry ➭ getMetricsAsJSON#1 with 64 is 68.92% faster.
✓ registry ➭ getMetricsAsJSON#2 with 8 is 110.6% faster.
✓ registry ➭ getMetricsAsJSON#2 with 4 and 2 with 2 is 72.82% faster.
✓ registry ➭ getMetricsAsJSON#2 with 2 and 2 with 4 is 481.3% faster.
✓ registry ➭ getMetricsAsJSON#6 with 2 is 127.6% faster.
✓ registry ➭ metrics#1 with 64 is 23.72% faster.
✓ registry ➭ metrics#2 with 8 is 53.88% faster.
✓ registry ➭ metrics#2 with 4 and 2 with 2 is 72.04% faster.
✓ registry ➭ metrics#2 with 2 and 2 with 4 is 71.24% faster.
✓ registry ➭ metrics#6 with 2 is 98.86% faster.
✓ histogram ➭ observe#1 with 64 is 1.422% faster.
✓ histogram ➭ observe#2 with 8 is 2.263% faster.
✓ histogram ➭ observe#2 with 4 and 2 with 2 is 5.482% faster.
⚠ histogram ➭ observe#2 with 2 and 2 with 4 is 1.523% acceptably slower.
✓ histogram ➭ observe#6 with 2 is 0.7490% faster.
✓ gauge ➭ inc is 11.81% faster.
✓ gauge ➭ inc with labels is 3.459% faster.
⚠ summary ➭ observe#1 with 64 is 3.768% acceptably slower.
⚠ summary ➭ observe#2 with 8 is 1.771% acceptably slower.
✓ summary ➭ observe#2 with 4 and 2 with 2 is 3.571% faster.
✓ summary ➭ observe#2 with 2 and 2 with 4 is 4.680% faster.
✓ summary ➭ observe#6 with 2 is 0.01691% faster.
prom-client benchmarks are super noisy... I don't think there's any reason gauge's perf should have changed, so these numbers are probably +/- at least 11.8%.
| for (const upperBound of histogram.upperBounds) { | ||
| const buckets = histogram.upperBounds.map(upperBound => { |
There was a problem hiding this comment.
Did you benchmark this change? Usually plain loops + arr.push() are faster than map()/forEach()/etc.
There was a problem hiding this comment.
See my previous comment: I checked changing the implementation of formatLabels to an explicit loop using for...of and it's indeed faster so I pushed this change. In the other cases map was either slightly faster for me, or the there was no noticeable difference, and so I took the shorter and more explicit implementation.
There was a problem hiding this comment.
Here are is the benchmark I'm getting:
✓ registry ➭ getMetricsAsJSON#1 with 64 is 85.08% faster.
✓ registry ➭ getMetricsAsJSON#2 with 8 is 61.45% faster.
✓ registry ➭ getMetricsAsJSON#2 with 4 and 2 with 2 is 245.8% faster.
✓ registry ➭ getMetricsAsJSON#2 with 2 and 2 with 4 is 136.1% faster.
✓ registry ➭ getMetricsAsJSON#6 with 2 is 223.3% faster.
✓ registry ➭ metrics#1 with 64 is 36.37% faster.
✓ registry ➭ metrics#2 with 8 is 50.48% faster.
✓ registry ➭ metrics#2 with 4 and 2 with 2 is 96.09% faster.
✓ registry ➭ metrics#2 with 2 and 2 with 4 is 72.52% faster.
✓ registry ➭ metrics#6 with 2 is 120.6% faster.
✓ histogram ➭ observe#1 with 64 is 0.5319% faster.
⚠ histogram ➭ observe#2 with 8 is 0.1193% acceptably slower.
✓ histogram ➭ observe#2 with 4 and 2 with 2 is 3.422% faster.
✓ histogram ➭ observe#2 with 2 and 2 with 4 is 2.720% faster.
✓ histogram ➭ observe#6 with 2 is 0.1891% faster.
✓ gauge ➭ inc is 4.797% faster.
✓ gauge ➭ inc with labels is 3.261% faster.
✓ summary ➭ observe#1 with 64 is 12.16% faster.
✓ summary ➭ observe#2 with 8 is 0.6340% faster.
⚠ summary ➭ observe#2 with 4 and 2 with 2 is 7.861% acceptably slower.
✓ summary ➭ observe#2 with 2 and 2 with 4 is 3.900% faster.
✓ summary ➭ observe#6 with 2 is 0.4984% faster.
There was a problem hiding this comment.
Usually plain loops +
arr.push()are faster thanmap()
I dispute this as a general statement: with map JS engine can allocated a properly sized array to begin with. OTOH when using push the engine may need to expand the size of the target array. Each such expansion means new allocation + copying over the values from the previous target array instance.
|
Any more thoughts / comments about this PR? Any chance that it will be merged? |
Cleans up and compacts the code.