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

fix: Use a pool of LabelPairs to reduce memory allocs #2838

Merged
merged 5 commits into from Dec 13, 2023

Conversation

bryanhuhta
Copy link
Contributor

@bryanhuhta bryanhuhta commented Dec 12, 2023

We see pretty heavy memory usage when querier.v1.QueryService/Series calls are made on blocks with large a index.tsdb. For a real world example, see here.

In this flamegraph, a ~7.7mb index.tsdb file is being used, resulting in using ~680mb of memory.

Screenshot 2023-12-12 at 4 26 51 PM

The culprit is this loop in getUniqueLabelsSets:

for postings.Next() {
matchedLabels := make(phlaremodel.Labels, 0, len(names))
for _, name := range names {
value, err := b.index.LabelValueFor(postings.At(), name)
if err != nil {
if err == storage.ErrNotFound {
continue
}
return nil, err
}
matchedLabels = append(matchedLabels, &typesv1.LabelPair{
Name: name,
Value: value,
})
}

The high memory usage comes from us allocating a slice every iteration and even more egregiously, allocating a new typesv1.LabelPair on the heap every time we have a label name/value match.

This PR changes getUniqueLabelsSets to use a slice pool strategy, where we allocate a pool of the exact size and fill it with pointers to heap memory. We reuse this pool instead of allocating new memory each iteration of postings.Next(). If we need the contents of the pool, we explicitly copy out of the pool. This substantially reduces memory usage.

After the change, the new flamegraph shows getUniqueLabelsSets using ~56mb of memory:

Screenshot 2023-12-12 at 4 27 05 PM

And some benchmarks using the same 7.7mb index.tsdb:

Name iterations ns/op B/op allocs/op
old 37 27478623 9061538 188762
new 39 26336128 1681138 83927
change -4.1% -81.4% -55.5%

Of course, now we have the problem of index.(*Reader).LabelValueFor using a lot of memory, but this is a significant and simple enough step that I think it's justifiable to merge on its own.

@bryanhuhta bryanhuhta self-assigned this Dec 12, 2023
@bryanhuhta bryanhuhta requested a review from a team as a code owner December 12, 2023 23:15
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM !!!!!!

@bryanhuhta bryanhuhta merged commit fba9246 into main Dec 13, 2023
19 checks passed
@bryanhuhta bryanhuhta deleted the fix-series-memory branch December 13, 2023 14:58
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.

None yet

2 participants