-
Notifications
You must be signed in to change notification settings - Fork 379
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
Mutate labels to avoid excessive object cloning. #220
Conversation
Thanks!
That's awesome, we've long wanted a benchmarking suite! But if we add it, I think it should be written in JS using e.g. https://github.com/bestiejs/benchmark.js/. |
@SimenB happy to do it in JS, I only used the one I added here because I already had it sitting around in another project, so just copy and pasted. I'll take on creating the benchmark suite using that library (or something similar). I will rip out the current suite and leave just the performance changes (you can feel free to test with the test script locally to confirm) |
@KevinAMurray linked to a suite written in the framework I suggested in https://github.com/KevinAMurray/prom-client/tree/master/benchmarks. Might be a good starting point 🙂 |
lib/registry.js
Outdated
}) | ||
); | ||
for (const val of item.values) { | ||
for (const label of Object.keys(this._defaultLabels)) { |
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.
Since this is a perf PR, we can lift Object.keys(this._defaultLabels)
out of the inner loop to avoid iterating through that object every time
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.
Definitely
With the latest benchmark suite and changes:
|
Oooh, I love the idea of requiring our own latest publish for comparison |
lib/histogram.js
Outdated
createBucketValues(bucketData, histogram) | ||
); | ||
const buckets = []; | ||
let acc = 0; |
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 inlined this to avoid creating a function on a big loop, also made performance profiling cleaner. If you don't like it I can revert, but I did get a performance gain by inlining the method.
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'm fine with this, especially if we now have benchmarks where it shows improvement
lib/histogram.js
Outdated
@@ -280,12 +280,13 @@ function convertLabelsAndValues(labels, value) { | |||
function extractBucketValuesForExport(histogram) { | |||
return bucketData => { | |||
const buckets = []; | |||
const bucketLabels = Object.entries(bucketData.labels); |
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.
Object.entries
is not available in Node 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.
Whoops! I'll switch back to just shifting Object.keys up. Sorry!
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.
no worries! I thought we had the linter set up to yell, apparently not...
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.
#221 fwiw 🙂
I switched to
|
|
It is not maintained (2016 was last commit) but it just extends |
Yeah, no worries. I'd rather think of it as "complete" and not "unmaintained" 😀 |
Mind updating the changelog? |
Sure thing! |
|
Great work! I'll try and have a closer look tonight |
lib/registry.js
Outdated
valAcc += '\n'; | ||
return valAcc; | ||
}, ''); | ||
values += line.join(' ').trim(); |
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.
values += line.join(' ').trim() + '\n';
to save an assignment
benchmarks/index.js
Outdated
} | ||
}) | ||
.on('complete', () => { | ||
// eslint-disable-next-line no-console |
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.
Instead of this, just add it as exception here:
Lines 64 to 69 in 25255c3
{ | |
"files": ["example/**/*.js"], | |
"rules": { | |
"no-console": "off" | |
} | |
} |
After
|
(not come across BenchTable -- very nice!) Benchmark is a great addition. And loving the performance increases. |
package.json
Outdated
@@ -39,6 +41,7 @@ | |||
"lint-staged": "^7.0.0", | |||
"lolex": "^2.1.3", | |||
"prettier": "1.14.2", | |||
"prom-client": "^11.1.2", |
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.
Love testing against a published version. 👍
This might be a non-issue, but we'll have to update this (and the lock files) after every release (or at least when we care about benchmarks). I don't see a way to exclude a module from the lock files, which would give some options for improving that situation. Not listing it as a dependency at all could be another option, and then we could put the require
for it in a try/catch?
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.
we could add a CI check or something? npm show prom-client version
returns latest published, and if the local version does not match, throw?
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, that is interesting. We could have a postinstall
step that only runs locally that installs the latest version? But yeah, I hadn't thought of the lockfile issue. Let me know what you prefer.
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.
Oo, using a package.json script is another good idea. Wouldn't want it to run all the time though, so if it's in postinstall
, would have to check somehow if it's being installed in a git checkout or as a dependency, I think?
(Might not be worth the trouble to make this "nice" -- could just manually bump it as necessary.)
Could you add some prose about the benchmarks to a |
lib/registry.js
Outdated
const values = (item.values || []).reduce((valAcc, val) => { | ||
const merged = Object.assign({}, this._defaultLabels, val.labels); | ||
help = `# HELP ${name} ${help}`; | ||
const type = `# TYPE ${name} ${item.type}`; |
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.
👏 that was long overdue!
lib/registry.js
Outdated
@@ -26,42 +26,51 @@ class Registry { | |||
} | |||
|
|||
getMetricAsPrometheusString(metric, conf) { | |||
const opts = Object.assign({}, defaultMetricsOpts, conf); |
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.
The point with using Object.assign like this was both a good way to extend and overwrite the default settings but also a way to make sure a specific configuration value always was there. I know that you changed on how you call this method from the metrics function but this function is public available so it is a breaking change.
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 can revert this change, not the biggest perf win. Good call on the public api change potential.
package.json
Outdated
"lolex": "^2.1.3", | ||
"prettier": "1.14.3", | ||
"prom-client": "^11.1.2", |
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 guess you could wildcard it? Doesn't solve the lock problem though.
I was planning on just making the benchmarking part of the |
I think HW varies too much for it to make sense to run as part of the test build. But I think we can paste the results of whatever we end up merging into a doc as a baseline |
The build will run not with the results of a previous run, but will run the two jobs (with published and local) so I don't think the HW matters, it will all be relative to the current HW compute power. If we were going based on previous results where it was on different hardware I would agree. Thoughts? |
Oh, like run against latest published, then current? That makes sense 🙂 |
SuccessFailure |
Haha, this is awesome! 😀 You might want to consider creating a module we can install for the setup part, but this looks really good. Thank you so much for working on it! |
Also, would you mind creating a separate PR for the benchmark (once fully iterated)? That will keep this PR more focused on perf improvements |
bin/check-versions.sh
Outdated
|
||
set -e | ||
|
||
INSTALLED_VERSION=`node -p -e "require('prom-client/package.json').version"` |
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.
you don't need the -e
, -p
is enough 🙂
😆I had a feeling I was approaching that boundary. Makes total sense, I just couldn't help myself. I will update this PR to just be the perf changes, and move the benchmark suite to a new PR.
Great idea! Once I open a PR with it separately, and land it, I will take on extracting it out into it's own package.
This is an awesome package. Thanks for helping to make it. |
a0087bb
to
709a407
Compare
I extracted the benchmark suite to #222 |
Thanks for the great review everyone. And thanks for the merge @siimon! Will you be releasing this as a new version, or waiting to batch it up with other changes? Thanks!!! |
|
Bug introduced at siimon#220 and fixed for .getMetricAsPrometheusString() at siimon#273
Bug introduced at siimon#220 and fixed for .getMetricAsPrometheusString() at siimon#273
I discovered another performance optimization we can perform, which is to mutate the
labels
and avoidObject.assign
where we can in hot paths.