Skip to content
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 encoding and decoding performance for native histograms in new internal query result payload format #4303

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Feb 27, 2023

What this PR does

This PR improves the performance of both encoding and decoding native histograms in the new internal query result payload format.

Benchmark results: encoding in querier
name                                                                              old time/op    new time/op    delta
ProtobufCodec_Encode/vector_with_series_with_histogram_value-10                      416ns ± 1%     349ns ± 0%  -16.03%  (p=0.008 n=5+5)
ProtobufCodec_Encode/vector_with_float_and_histogram_values-10                       837ns ± 1%     716ns ± 0%  -14.40%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_histogram_value-10                      466ns ± 2%     426ns ± 0%   -8.46%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_both_float_and_histogram_values-10      716ns ± 0%     649ns ± 1%   -9.32%  (p=0.016 n=4+5)

name                                                                              old alloc/op   new alloc/op   delta
ProtobufCodec_Encode/vector_with_series_with_histogram_value-10                       456B ± 0%      424B ± 0%   -7.02%  (p=0.008 n=5+5)
ProtobufCodec_Encode/vector_with_float_and_histogram_values-10                      1.06kB ± 0%    1.00kB ± 0%   -6.02%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_histogram_value-10                       544B ± 0%      496B ± 0%   -8.82%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_both_float_and_histogram_values-10       944B ± 0%      848B ± 0%  -10.17%  (p=0.008 n=5+5)

name                                                                              old allocs/op  new allocs/op  delta
ProtobufCodec_Encode/vector_with_series_with_histogram_value-10                       7.00 ± 0%      5.00 ± 0%  -28.57%  (p=0.008 n=5+5)
ProtobufCodec_Encode/vector_with_float_and_histogram_values-10                        13.0 ± 0%       9.0 ± 0%  -30.77%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_histogram_value-10                       8.00 ± 0%      6.00 ± 0%  -25.00%  (p=0.008 n=5+5)
ProtobufCodec_Encode/matrix_with_series_with_both_float_and_histogram_values-10       11.0 ± 0%       7.0 ± 0%  -36.36%  (p=0.008 n=5+5)
Benchmark results: decoding in query-frontend
name                                                                                                            old time/op    new time/op    delta
ProtobufFormat_DecodeResponse/successful_vector_response_with_histogram_value-10                                  2.56µs ± 0%    2.51µs ± 1%  -1.80%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_vector_response_with_float_and_histogram_values-10                       2.77µs ± 0%    2.75µs ± 0%  -0.46%  (p=0.040 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_histogram_value-10                                  2.73µs ± 1%    2.67µs ± 0%  -2.49%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_float_and_histogram_values-10                       2.76µs ± 1%    2.67µs ± 1%  -3.14%  (p=0.008 n=5+5)

name                                                                                                            old alloc/op   new alloc/op   delta
ProtobufFormat_DecodeResponse/successful_vector_response_with_histogram_value-10                                  2.97kB ± 0%    2.94kB ± 0%  -1.08%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_vector_response_with_float_and_histogram_values-10                       3.18kB ± 0%    3.14kB ± 0%  -1.01%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_histogram_value-10                                  3.12kB ± 0%    3.07kB ± 0%  -1.54%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_float_and_histogram_values-10                       3.15kB ± 0%    3.10kB ± 0%  -1.52%  (p=0.008 n=5+5)

name                                                                                                            old allocs/op  new allocs/op  delta
ProtobufFormat_DecodeResponse/successful_vector_response_with_histogram_value-10                                    50.0 ± 0%      48.0 ± 0%  -4.00%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_vector_response_with_float_and_histogram_values-10                         57.0 ± 0%      55.0 ± 0%  -3.51%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_histogram_value-10                                    54.0 ± 0%      52.0 ± 0%  -3.70%  (p=0.008 n=5+5)
ProtobufFormat_DecodeResponse/successful_matrix_response_with_float_and_histogram_values-10                         56.0 ± 0%      54.0 ± 0%  -3.57%  (p=0.008 n=5+5)

Note that it requires a breaking change to the protobuf schema, but I believe this is acceptable given native histograms are an experimental feature and we're not yet running it in production.

Which issue(s) this PR fixes or relates to

#4104

Checklist

  • Tests updated
  • [n/a] Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@charleskorn charleskorn marked this pull request as ready for review February 27, 2023 02:00
@charleskorn charleskorn requested a review from a team as a code owner February 27, 2023 02:00
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change makes sense to me. I let @krajorama review it too before merging.

Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, also we could reuse the requireSameShape function for other structs we have in there

@charleskorn charleskorn merged commit 96f35b8 into sparsehistogram Feb 27, 2023
@charleskorn charleskorn deleted the charleskorn/improve-histogram-encode-decode-performance branch February 27, 2023 22:36
charleskorn added a commit that referenced this pull request Mar 3, 2023
This does not include the associated integration test changes, or
changes to tests that have not yet been merged to main.
krajorama pushed a commit that referenced this pull request Mar 7, 2023
…istogram` onto `main` (#4360)

* Cherry pick changes from #4269, #4303 and #4318 onto main.

This does not include the associated integration test changes, or
changes to tests that have not yet been merged to main.

* Add support for JSON-encoding a single series with both float and histogram values.

Co-authored-by: Ganesh Vernekar <ganeshvern@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants