chore: Memory improvements in dataobj page downloads #19301
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rfratto
reviewed
Sep 30, 2025
pkg/util/rangeio/rangeio.go
Outdated
Comment on lines
271
to
273
| for _, r := range optimized { | ||
| putBytesBuffer(&r.Data) | ||
| } |
There was a problem hiding this comment.
I believe this still allocates because it's taking the address of the struct field and not actually the byte slice.
As an alternative, you can:
- Use a pool of
*bytes.Bufferwhere you use their raw byte slices - Update
optimizedRangesto return arelease func()which returns the original buffers back to the pool.
Then you'd have
optimized, release := optimizedRanges(cfg, ranges)
defer release() This is what I did in my hack, and it worked out well.
The only issue is that it's a little annoying to get the slice from *bytes.Buffer, I had to do this:
size := coalescedChunks[i].Length
// TODO: Release but back into the
// pool in the returned release func
buf := bytesBufferPool.Get().(*bytes.Buffer)
buf.Reset()
buf.Grow(size)
out[i] = Range{
Data: buf.Bytes()[:size],
Offset: coalescedChunks[i].Offset,
}There was a problem hiding this comment.
Nice, thats definitely cleaner. I played around with bytes.Buffer originally, but I was returning the byte slice too soon so wasn't able to track when it grew. Thanks for the tip.
Here are the new benchmark results:
goos: darwin
goarch: arm64
pkg: github.com/grafana/loki/v3/pkg/logql/bench
cpu: Apple M3 Max
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 10.43 ± 5% 10.18 ± 3% -2.48% (p=0.019 n=10)
│ before.txt │ after.txt │
│ kilobytesProcessed │ kilobytesProcessed vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 52.87k ± 0% 52.87k ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ before.txt │ after.txt │
│ linesProcessed │ linesProcessed vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 6.233M ± 0% 6.233M ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ before.txt │ after.txt │
│ postFilterLines │ postFilterLines vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
¹ all samples are equal
│ before.txt │ after.txt │
│ B/op │ B/op vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 1022.1Mi ± 6% 730.1Mi ± 18% -28.57% (p=0.000 n=10)
│ before.txt │ after.txt │
│ allocs/op │ allocs/op vs base │
LogQL/query=sum_by_(cluster,_namespace)_(count_over_time({service_name="grafana",_env="prod",_region="us-west-2"}_|=_"level"_[1m0s]))/kind=metric/store=dataobj-engine-14 2.416M ± 0% 2.415M ± 0% ~ (p=0.796 n=10)
This comment has been minimized.
This comment has been minimized.
rfratto
approved these changes
Oct 1, 2025
pkg/util/rangeio/rangeio.go
Outdated
| i += 2 // Skip over the range we just inserted. | ||
| } | ||
|
|
||
| usedBuffers := []*bytes.Buffer{} |
There was a problem hiding this comment.
nit: you could preallocate this to the target capacity (or length) since it'll have the same number of elements as out
|
😢 zizmor failed with exit code 14. Expand for full output |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
A small optimization to use page compressed size for downloading page data (this didn't do much on the LogQL suite)
Introduced a basic buffer pool mechanism to rangeio's optimized requests, which resulted in a 30-40% reduction in bytes allocated during the benchmarks.