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

Use labels.ScratchBuilder to produce aggregate series #294

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

fpetkovski
Copy link
Collaborator

@fpetkovski fpetkovski commented Jul 11, 2023

This commit updates aggregate operators to use the newly introduced labels.ScratchBuilder from Prometheus when calculating output series.

Showing only differences in the memory benchmark. It looks like there's a measurable difference for aggregates with grouping labels.

name                                                  old alloc/op   new alloc/op   delta
RangeQuery/sum_by_pod-8                                 18.7MB ± 0%    18.1MB ± 0%  -3.39%  (p=0.008 n=5+5)
RangeQuery/topk-8                                       9.61MB ± 0%    9.59MB ± 0%  -0.19%  (p=0.032 n=5+5)
RangeQuery/rate-8                                       30.4MB ± 0%    30.4MB ± 0%  +0.04%  (p=0.032 n=5+5)
RangeQuery/sum_by_rate-8                                20.1MB ± 0%    19.5MB ± 0%  -2.94%  (p=0.008 n=5+5)
RangeQuery/quantile_with_variable_parameter-8           34.0MB ± 0%    33.3MB ± 0%  -1.90%  (p=0.008 n=5+5)
RangeQuery/at_modifier_-8                               23.9MB ± 0%    23.9MB ± 0%  -0.01%  (p=0.016 n=5+5)
RangeQuery/at_modifier_with_positive_offset_vector-8    23.7MB ± 0%    23.7MB ± 0%  -0.01%  (p=0.008 n=5+5)
RangeQuery/complex_func_query-8                         31.7MB ± 0%    31.8MB ± 0%  +0.24%  (p=0.008 n=5+5)
RangeQuery/sort-8                                       28.3MB ± 0%    28.2MB ± 0%  -0.07%  (p=0.032 n=5+5)

It also adjusts all operators to make sure they set the pool buffer size before allocating output samples.

@fpetkovski
Copy link
Collaborator Author

I caught the cases where we don't set output buffer sizes by panicking in the pool if the size is 0. Maybe we can have an implementation of the pool which does this and use it in tests?

This commit updates aggregate operators to use the newly introduced
labels.ScratchBuilder from Prometheus when calculating output series.

It also adjusts all operators to make sure they set the pool buffer size
before allocating output samples.

Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

LGTM!

@fpetkovski fpetkovski merged commit 671d55a into thanos-io:main Jul 19, 2023
@GiedriusS
Copy link
Member

GiedriusS commented Jul 19, 2023

671d55a#commitcomment-122044835 seems like this degraded performance? 🤔 actually, just one run's CPU usage jumped so perhaps this was a flake

@fpetkovski
Copy link
Collaborator Author

Yeah I also saw that, it's pretty cool that the CI job can report this.

I will take a look at the change again to see if is legit.

@MichaHoffmann
Copy link
Contributor

Whaaa thast pretty big regression for absent

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.

4 participants