Improve performance of hashObject#594
Conversation
|
Thanks for the PR. This has been proposed before in #373 and would make it impossible to implement #298/#368. See also this comment about validly accepting subsets of labels: #373 (comment). |
|
@zbjornson thanks for the feedback. First regarding the tests example in the comments: const prom = require('.');
async function test1() {
instance = new prom.Gauge({
name: 'name_2',
help: 'help',
labelNames: ['code', 'success'],
});
instance.inc({ code: '200' }, 10);
const str = await prom.Registry.globalRegistry.getSingleMetricAsString('name_2');
console.log(str);
}
async function test2() {
instance = new prom.Gauge({
name: 'test',
help: 'help',
labelNames: ['code', 'success', 'ok'],
});
instance.inc({ code: '200' }, 10);
instance.inc({ code: '200', ok: 'yes' }, 10);
instance.inc({ ok: 'yes', success: 'false' }, 10);
instance.inc({ success: 'false', ok: 'no' }, 10);
const str = await prom.Registry.globalRegistry.getSingleMetricAsString('test');
console.log(str);
}
async function main() {
await test1();
console.log("\n\n");
await test2();
}
main();I am getting this output: Which is the exact same output as master branch. Regarding support of dynamic labels, I have strong opinion against this, since it can lead to a mess in metrics on large-scale codebases and having labels predefined gives more strictness. Please re-consider this change, and I am happy to get more feedback here. |
0512e46 to
1270f33
Compare
My bad, I missed the fallback. That's a great solution then. Can you add a changelog entry please? |
lib/util.js
Outdated
|
|
||
| let keys = Object.keys(labels); | ||
| if (keys.length > 1) { | ||
| keys = keys.sort(); // need consistency across calls |
There was a problem hiding this comment.
If you're making other changes, you could remove the useless keys = bit for clarity.
There was a problem hiding this comment.
removed the assignment.
|
@zbjornson added changelog entry, let me know if it's ok. |
lib/util.js
Outdated
| for (let i = 0; i < keys.length; i++) { | ||
| const key = keys[i]; | ||
| const value = labels[key]; | ||
| if (typeof value === 'undefined') continue; |
There was a problem hiding this comment.
is typeof value === 'undefined' faster than just value === undefined? One cannot redefine undefined in a strict context afaik, so it shouldn't be needed to workaround
There was a problem hiding this comment.
I think it's hard in benchmarks to see the difference between the two options here, I think it falls within a margin of error.
I ran the benchmarks with typeof and === undefined and I am getting sometimes that === undefined is faster and sometimes it's slower. It's probably because I am running the benchmarks on my laptop and there is lots of variance.
I looked at difference of v8 bytecode and looks like === undefined have optimized bytecode:
typeof x === 'undefined'
0x222300199ff0 @ 40 : 20 05 TestTypeOf #5
0x222300199ff2 @ 42 : 9a 04 JumpIfFalse [4] (0x222300199ff6 @ 46)
0x222300199ff4 @ 44 : 8b 1f Jump [31] (0x22230019a013 @ 75)
x === undefined
0x2c0a00199ff0 @ 40 : 9e 04 JumpIfNotUndefined [4] (0x2c0a00199ff4 @ 44)
And I see that lodash is doing === undefined.
So I am changing to === undefined check.
Since label names are predefined ahead-of-time and constant during runtime, we can sort them before-hand and use them when running `hashObject. node v18.18.2 ``` http fetch GET 200 https://registry.npmjs.org/prom-client 83ms http fetch GET 200 https://registry.npmjs.org/prom-client/-/prom-client-15.0.0.tgz 42ms + prom-client@15.0.0 added 4 packages from 4 contributors and audited 4 packages in 0.382s found 0 vulnerabilities - histogram ➭ observe#1 with 64 ➭ current x 104,381 ops/sec ±0.42% (99 runs sampled) - histogram ➭ observe#1 with 64 ➭ prom-client@latest x 103,709 ops/sec ±0.72% (96 runs sampled) - histogram ➭ observe#2 with 8 ➭ current x 68,178 ops/sec ±0.20% (98 runs sampled) - histogram ➭ observe#2 with 8 ➭ prom-client@latest x 53,510 ops/sec ±0.18% (97 runs sampled) - histogram ➭ observe#2 with 4 and 2 with 2 ➭ current x 34,169 ops/sec ±0.22% (97 runs sampled) - histogram ➭ observe#2 with 4 and 2 with 2 ➭ prom-client@latest x 28,170 ops/sec ±0.32% (97 runs sampled) - histogram ➭ observe#2 with 2 and 2 with 4 ➭ current x 36,057 ops/sec ±0.36% (97 runs sampled) - histogram ➭ observe#2 with 2 and 2 with 4 ➭ prom-client@latest x 28,804 ops/sec ±0.20% (98 runs sampled) - histogram ➭ observe#6 with 2 ➭ current x 23,360 ops/sec ±0.14% (97 runs sampled) - histogram ➭ observe#6 with 2 ➭ prom-client@latest x 19,420 ops/sec ±0.36% (96 runs sampled) - counter ➭ inc ➭ current x 59,643,643 ops/sec ±0.13% (101 runs sampled) - counter ➭ inc ➭ prom-client@latest x 37,023,723 ops/sec ±0.46% (95 runs sampled) - counter ➭ inc with labels ➭ current x 84,762 ops/sec ±0.25% (97 runs sampled) - counter ➭ inc with labels ➭ prom-client@latest x 67,225 ops/sec ±0.62% (95 runs sampled) - gauge ➭ inc ➭ current x 61,652,788 ops/sec ±0.41% (96 runs sampled) - gauge ➭ inc ➭ prom-client@latest x 31,247,588 ops/sec ±0.28% (98 runs sampled) - gauge ➭ inc with labels ➭ current x 86,552 ops/sec ±0.54% (97 runs sampled) - gauge ➭ inc with labels ➭ prom-client@latest x 64,590 ops/sec ±0.47% (95 runs sampled) - summary ➭ observe#1 with 64 ➭ current x 87,909 ops/sec ±0.18% (98 runs sampled) - summary ➭ observe#1 with 64 ➭ prom-client@latest x 94,806 ops/sec ±0.87% (89 runs sampled) - summary ➭ observe#2 with 8 ➭ current x 61,909 ops/sec ±0.15% (99 runs sampled) - summary ➭ observe#2 with 8 ➭ prom-client@latest x 49,337 ops/sec ±0.35% (100 runs sampled) - summary ➭ observe#2 with 4 and 2 with 2 ➭ current x 32,320 ops/sec ±0.20% (100 runs sampled) - summary ➭ observe#2 with 4 and 2 with 2 ➭ prom-client@latest x 27,597 ops/sec ±0.79% (95 runs sampled) - summary ➭ observe#2 with 2 and 2 with 4 ➭ current x 32,187 ops/sec ±0.21% (100 runs sampled) - summary ➭ observe#2 with 2 and 2 with 4 ➭ prom-client@latest x 27,513 ops/sec ±0.37% (93 runs sampled) - summary ➭ observe#6 with 2 ➭ current x 22,375 ops/sec ±0.47% (94 runs sampled) - summary ➭ observe#6 with 2 ➭ prom-client@latest x 19,161 ops/sec ±0.14% (100 runs sampled) ┌───────────────────────────────┬────────────────────┬────────────────────┐ │ histogram │ current │ prom-client@latest │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#1 with 64 │ 104381.0327549696 │ 103709.41386205897 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 8 │ 68178.1543735591 │ 53509.94862568404 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 4 and 2 with 2 │ 34169.376174278514 │ 28170.10920967872 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 2 and 2 with 4 │ 36056.509189653516 │ 28804.04102513799 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#6 with 2 │ 23359.579524196415 │ 19419.640968121184 │ └───────────────────────────────┴────────────────────┴────────────────────┘ ┌─────────────────┬───────────────────┬────────────────────┐ │ counter │ current │ prom-client@latest │ ├─────────────────┼───────────────────┼────────────────────┤ │ inc │ 59643642.79575977 │ 37023723.28415886 │ ├─────────────────┼───────────────────┼────────────────────┤ │ inc with labels │ 84761.87024308582 │ 67224.57758629428 │ └─────────────────┴───────────────────┴────────────────────┘ ┌─────────────────┬───────────────────┬────────────────────┐ │ gauge │ current │ prom-client@latest │ ├─────────────────┼───────────────────┼────────────────────┤ │ inc │ 61652788.35302279 │ 31247587.722701166 │ ├─────────────────┼───────────────────┼────────────────────┤ │ inc with labels │ 86551.57212390135 │ 64589.60038388497 │ └─────────────────┴───────────────────┴────────────────────┘ ┌───────────────────────────────┬────────────────────┬────────────────────┐ │ summary │ current │ prom-client@latest │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#1 with 64 │ 87908.71972414361 │ 94806.18075509806 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 8 │ 61909.18982275139 │ 49336.623224028765 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 4 and 2 with 2 │ 32319.8211562837 │ 27596.72566017053 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#2 with 2 and 2 with 4 │ 32187.468085238048 │ 27513.002743185254 │ ├───────────────────────────────┼────────────────────┼────────────────────┤ │ observe#6 with 2 │ 22375.38970678346 │ 19160.867264237862 │ └───────────────────────────────┴────────────────────┴────────────────────┘ ✓ histogram ➭ observe#1 with 64 is 0.6476% faster. ✓ histogram ➭ observe#2 with 8 is 27.41% faster. ✓ histogram ➭ observe#2 with 4 and 2 with 2 is 21.30% faster. ✓ histogram ➭ observe#2 with 2 and 2 with 4 is 25.18% faster. ✓ histogram ➭ observe#6 with 2 is 20.29% faster. ✓ counter ➭ inc is 61.10% faster. ✓ counter ➭ inc with labels is 26.09% faster. ✓ gauge ➭ inc is 97.30% faster. ✓ gauge ➭ inc with labels is 34.00% faster. ⚠ summary ➭ observe#1 with 64 is 7.846% acceptably slower. ✓ summary ➭ observe#2 with 8 is 25.48% faster. ✓ summary ➭ observe#2 with 4 and 2 with 2 is 17.11% faster. ✓ summary ➭ observe#2 with 2 and 2 with 4 is 16.99% faster. ✓ summary ➭ observe#6 with 2 is 16.78% faster. ```
19bc8d3 to
aa7ec23
Compare
|
@zbjornson @SimenB after addressing your questions & concerns, can we move forward with this PR? or there is something left here? |
zbjornson
left a comment
There was a problem hiding this comment.
Looks great, thanks for the contribution. Sorry again for the off-base review earlier.
Since label names are predefined ahead-of-time and constant during
runtime, we can sort them before-hand and use them when running
hashObject.Benchmarks
Running on node v18.18.2.
Note: the registry benchmarks I couldn't run because of OOM (and on master branch as well).