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

Optimize aggregate accumulators #323

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

fpetkovski
Copy link
Collaborator

@fpetkovski fpetkovski commented Nov 3, 2023

Scalar tables used in the hash aggregate with grouping labels are currently very wasteful because of the dynamic functions they create for each output group. Each function ends up as a new object on the heap, so for queries where output groups have high cardinality we end up using a lot of memory.

This commit fixes that by converting accumulators to structs, so that functions between accumulator instances do not end up as heap-allocated objects.

Here is an extreme case where accumulators took 12GB of heap space:
image

@fpetkovski fpetkovski force-pushed the optimize-accumulators branch from 9704319 to d035a9a Compare November 3, 2023 13:45
Scalar tables used for the hash aggregate with grouping labels are
currently very wasteful because of the dynamic functions they create
for each output group. Each function ends up as a new object on the heap,
so for queries where output groups have high cardinality we end up using
a lot of memory.

This commit fixes that by converting accumulators to structs, so that
functions between accumulator instances do not end up as heap-allocated objects.

Signed-off-by: Filip Petkovski <[email protected]>
@fpetkovski fpetkovski force-pushed the optimize-accumulators branch from d035a9a to a005b25 Compare November 3, 2023 13:52
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
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@yeya24 yeya24 merged commit fcf8381 into thanos-io:main Nov 4, 2023
6 checks passed
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