Skip to content

fix: avoid copying label values from tsdb unless required #17077

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

sandeepsukhani
Copy link
Contributor

@sandeepsukhani sandeepsukhani commented Apr 9, 2025

What this PR does / why we need it:
We are seeing OOMKills on index gateways due to a huge amount of allocations from LabelValues method of tsdb index reader. It happens due to copying all the label values read from the tsdb. This problem arises when the label matchers have any negative matchers.

I have changed the code to avoid making a copy of the label values unless we want to send it back as a response. I have checked all the usages of that method, and the only place we need to copy the values is when the LabelValues method is called explicitly to get all the values.

Here is the benchmark showing the difference in performance:

benchmark                              old ns/op      new ns/op     delta
BenchmarkTSDBIndex_GetChunkRefs-10     875881104      927752604     +5.92%
BenchmarkTSDBIndex_GetChunkRefs-10     879311146      862689354     -1.89%
BenchmarkTSDBIndex_GetChunkRefs-10     881187479      926149750     +5.10%
BenchmarkTSDBIndex_GetChunkRefs-10     910079084      899049584     -1.21%
BenchmarkTSDBIndex_GetChunkRefs-10     1014711583     911985770     -10.12%

benchmark                              old allocs     new allocs     delta
BenchmarkTSDBIndex_GetChunkRefs-10     2000140        1000154        -50.00%
BenchmarkTSDBIndex_GetChunkRefs-10     2000139        1000151        -50.00%
BenchmarkTSDBIndex_GetChunkRefs-10     2000141        1000150        -50.00%
BenchmarkTSDBIndex_GetChunkRefs-10     2000141        1000149        -50.00%
BenchmarkTSDBIndex_GetChunkRefs-10     2000146        1000153        -50.00%

benchmark                              old bytes     new bytes     delta
BenchmarkTSDBIndex_GetChunkRefs-10     176039840     168119396     -4.50%
BenchmarkTSDBIndex_GetChunkRefs-10     176039752     168118520     -4.50%
BenchmarkTSDBIndex_GetChunkRefs-10     176039944     168118504     -4.50%
BenchmarkTSDBIndex_GetChunkRefs-10     176039952     168118408     -4.50%
BenchmarkTSDBIndex_GetChunkRefs-10     176057848     168119296     -4.51%

Special notes for your reviewer:
I introduced the copying of label values in PR #7502 to avoid corruption of values when the underlying tsdb is closed.

@sandeepsukhani sandeepsukhani requested a review from a team as a code owner April 9, 2025 08:33
@sandeepsukhani sandeepsukhani enabled auto-merge (squash) April 9, 2025 08:47
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

@sandeepsukhani sandeepsukhani merged commit 8e4b104 into main Apr 9, 2025
62 checks passed
@sandeepsukhani sandeepsukhani deleted the tsdb-avoid-copying-label-values branch April 9, 2025 09:24
@sandeepsukhani sandeepsukhani added type/bug Somehing is not working as expected backport k249 labels Apr 9, 2025
loki-gh-app bot pushed a commit that referenced this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport k249 size/M type/bug Somehing is not working as expected
2 participants