chore(dataobj): Improve performance of dataset.Value API #18339
Merged
+247
−152
Conversation
This file contains hidden or 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
Using dataset.Value is a sizeable portion of total query time. This commit adds benchmarks to more directly test improvements to dataset.Value.
This commit makes several small changes to improve the performance of the dataset.Value API: 1. Reduce the complexity of methods so that methods can be inlined. The previous complexity of Value.Type bubbled down to all the methods, making them hard to inline. Using error types for panics also reduces the complexity cost of the panic lines while retaining the same error messages; the cost of both string concatenation and fmt.Sprintf bubbled up the cost, further making it difficult to inline functions. 2. Use pointer receivers wherever possible for Value methods. As each Value is 28 bytes (previously 24, but still large), copying the Value on the stack to call a non-inlined method was expensive. This was seen most prominently in CompareValues, which still can't be inlined. Using pointers for CompareValues significantly improves its performance. 3. Replace use of `any` for the final field with a plain `*byte` type. This ensures that there are absolutely no allocations for that field. Some usages of the old field have been replaced with the explicit kind field. Additionally, smaller changes have been made: * All methods that mutate the state of Value have been removed, with the exception of Zero. Functionality of these methods have been pushed down to the caller. This was done to reduce the complexity of the code, but also to manually inline that logic into the plain value encoder. * Usage of cmp.Compare has been replaced with a hand-rolled compare method for integers, which is slightly less expensive due to not needing to handle the possibility of floating-point numbers. As a result of these changes, the synthetic logql/bench benchmarks show a global 2x speed improvement in queries, alongside a 4% reduction in allocations.
|
I made a few smaller changes since the last time I ran the benchmarks. I'm running them one final time, and I'll share the results here. |
|
The results are a little hard to verify using the CI, since it seems pretty inconsistent with its results today. It initially looked like this PR improved the speed by 2x, but it seems like it's probably closer to 10-15%. The correctness tests are failing too, but they are also failing on the baseline I used, so I think that can be ignored here. |
ashwanthgoli
reviewed
Jul 8, 2025
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
andrejshapal
pushed a commit
to andrejshapal/loki
that referenced
this pull request
Jul 8, 2025
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
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.
This PR makes several small changes to improve the performance of the dataset.Value API:
Reduce the complexity of methods so that methods can be inlined. The previous complexity of Value.Type bubbled down to all the methods, making them hard to inline.
Using error types for panics also reduces the complexity cost of the panic lines while retaining the same error messages; the cost of both string concatenation and fmt.Sprintf bubbled up the cost, further making it difficult to inline functions.
Use pointer receivers wherever possible for Value methods. As each Value is 28 bytes (previously 24, but still large), copying the Value on the stack to call a non-inlined method was expensive. This was seen most prominently in CompareValues, which still can't be inlined. Using pointers for CompareValues significantly improves its performance.
Replace use of
anyfor the final field with a plain*bytetype. This ensures that there are absolutely no allocations for that field. Some usages of the old field have been replaced with the explicit kind field.Additionally, smaller changes have been made:
All methods that mutate the state of Value have been removed, with the exception of Zero. Functionality of these methods have been pushed down to the caller. This was done to reduce the complexity of the code, but also to manually inline that logic into the plain value encoder.
Usage of cmp.Compare has been replaced with a hand-rolled compare method for integers, which is slightly less expensive due to not needing to handle the possibility of floating-point numbers.
As a result of these changes, the synthetic logql/bench benchmarks show a 10-15% speed improvement in all queries.