perf: improve perf by caching unchanged histogram strings between gets #555
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This conflicts with #549 - but the optimisations here might've been applied there? |
|
Is that what you prefer? That I apply my optimizations on top of that PR? Or to wait until it's merged and then review/merge this PR separately? Also, can you review and provide feedback on this in any event, even if you don't want to merge yet? |
|
#549 has landed on master, which is why this PR has conflicts. If the optimisations in this PR are still applicable, I'd love to land them 🙂 The changes in this PR in isolation looks good 👍 |
|
Checking to determine if still applicable. In the interim closing this PR |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Histogram.get()returns the content of the histogram as JSON that gets converted into a string representation byRegistry.getMetricsAsString(). This conversion can be expensive, especially when there are many labels / label values / buckets, because each combination needs to be converted. This overhead can be reduced by caching the string representations for each entry in the histogram hash. This is because when there are many combinations, it's likely that most won't change between getting the metrics.In order to perform the caching, the conversion to string needs to happen inside
Histogram.get(). To accomplish thisRegistry.getMetricsAsString()will now pass into it an optional value-to-string conversion function. If such a function is provided then the caching will happen, otherwise the same behavior as before.It's possible to easily extend this behavior to the other metrics types. However they won't get such a significant improvement as the conversion operation for them is less expensive per value.
note: registry benchmark has been modified to perform an update of an eighth if a histogram between getting the metrics string. This is so that it's not all cached. This is because full caching is so fast the benchmark explodes.