-
Notifications
You must be signed in to change notification settings - Fork 378
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 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
} | ||
// else | ||
|
||
let keys = Object.keys(labels); | ||
if (keys.length > 1) { | ||
keys = keys.sort(); // need consistency across calls |
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.
If you're making other changes, you could remove the useless keys =
bit for clarity.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
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.
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).