Skip to content

perf: Fix memory leak in cachedIterator - #17628

Merged
benclive merged 1 commit into
mainfrom
benclive/fix-mem-leak-in-iterator
Jun 13, 2025
Merged

perf: Fix memory leak in cachedIterator#17628
benclive merged 1 commit into
mainfrom
benclive/fix-mem-leak-in-iterator

Conversation

@benclive

@benclive benclive commented May 9, 2025

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:
Fixes closing the wrapper iterator in cachedIterators when Close is called.

  • I was looking into the lz4-based memory leak reported in an issue, and noticed we only leak memory when running FORWARD queries and not BACKWARD queries.
  • This cached iterator is often only used for FORWARD queries, so we don't see this leak in our day-to-day as we usually only run BACKWARD queries frequently
  • I've attached a benchstat after making this change. Mostly unchanged results except much lower bytes allocated on FORWARD queries.
goos: darwin
goarch: arm64
pkg: github.com/grafana/loki/v3/pkg/logql/bench
cpu: Apple M3 Max
                                                                                    │ before_fix.txt │            after_fix.txt            │
                                                                                    │     sec/op     │   sec/op     vs base                │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14      228.4m ± 1%   235.7m ± 4%   +3.18% (p=0.000 n=10)
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14       49.46m ± 1%   34.29m ± 6%  -30.67% (p=0.000 n=10)
geomean                                                                                  106.3m        89.90m       -15.42%

                                                                                    │   before_fix.txt   │                after_fix.txt                │
                                                                                    │ kilobytesProcessed │ kilobytesProcessed  vs base                 │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14          12.10k ± 0%          12.10k ± 0%       ~ (p=1.000 n=10) ¹
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14            362.7 ± 0%           362.7 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                                                      2.095k               2.095k       +0.00%
¹ all samples are equal

                                                                                    │ before_fix.txt │              after_fix.txt              │
                                                                                    │ linesProcessed │ linesProcessed  vs base                 │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14      35.59k ± 0%      35.59k ± 0%       ~ (p=1.000 n=10) ¹
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14       1.070k ± 0%      1.070k ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                                                  6.171k           6.171k       +0.00%
¹ all samples are equal

                                                                                    │ before_fix.txt  │              after_fix.txt               │
                                                                                    │ postFilterLines │ postFilterLines  vs base                 │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14       35.59k ± 0%       35.59k ± 0%       ~ (p=1.000 n=10) ¹
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14        1.070k ± 0%       1.070k ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                                                   6.171k            6.171k       +0.00%
¹ all samples are equal

                                                                                    │ before_fix.txt │             after_fix.txt             │
                                                                                    │      B/op      │     B/op       vs base                │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14    218.2Mi ± 10%   214.3Mi ± 10%        ~ (p=1.000 n=10)
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14     608.8Mi ±  1%   105.0Mi ±  2%  -82.75% (p=0.000 n=10)
geomean                                                                                364.5Mi         150.0Mi        -58.84%

                                                                                    │ before_fix.txt │           after_fix.txt            │
                                                                                    │   allocs/op    │  allocs/op   vs base               │
LogQL/query={region="ap-southeast-1",_env="dev"}_[BACKWARD]/kind=log/store=chunk-14      725.1k ± 0%   725.1k ± 0%       ~ (p=0.971 n=10)
LogQL/query={region="ap-southeast-1",_env="dev"}_[FORWARD]/kind=log/store=chunk-14       421.8k ± 0%   421.1k ± 0%  -0.16% (p=0.000 n=10)
geomean                                                                                  553.0k        552.6k       -0.08%

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • Title matches the required conventional commits format, see here
    • Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
@benclive
benclive requested a review from a team as a code owner May 9, 2025 18:09
@benclive
benclive merged commit 87aa954 into main Jun 13, 2025
@benclive
benclive deleted the benclive/fix-mem-leak-in-iterator branch June 13, 2025 08:20
chaudum added a commit that referenced this pull request Aug 1, 2025
ashwanthgoli added a commit that referenced this pull request Aug 1, 2025
loki-gh-app Bot pushed a commit that referenced this pull request Aug 1, 2025
loki-gh-app Bot pushed a commit that referenced this pull request Aug 1, 2025
ashwanthgoli added a commit that referenced this pull request Aug 1, 2025
…ort k265) (#18689)

Co-authored-by: Ashwanth <iamashwanth@gmail.com>
chaudum added a commit that referenced this pull request Aug 1, 2025
…ort k264) (#18688)

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Co-authored-by: Ashwanth <iamashwanth@gmail.com>
Co-authored-by: Christian Haudum <christian.haudum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants