Skip to content

Optimize ts v2 pool #7373

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.

Sign up for GitHub

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

Open
SungJin1212 wants to merge 1 commit into
base: master
Choose a base branch
from
+37 −0
Open

Optimize ts v2 pool #7373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* [BUGFIX] Fix nil when ingester_query_max_attempts > 1. #7369
* [BUGFIX] Querier: Fix queryWithRetry and labelsWithRetry returning (nil, nil) on cancelled context by propagating ctx.Err(). #7370
* [BUGFIX] Metrics Helper: Fix non-deterministic bucket order in merged histograms by sorting buckets after map iteration, matching Prometheus client library behavior. #7380
* [BUGFIX] Fix memory leak in `ReuseWriteRequestV2` by explicitly clearing the `Symbols` backing array string pointers before returning the object to `sync.Pool`. #7373
* [BUGFIX] Distributor: Return HTTP 401 Unauthorized when tenant ID resolution fails in the Prometheus Remote Write 2.0 path. #7389

## 1.21.0 in progress
Expand Down
4 changes: 4 additions & 0 deletions pkg/cortexpb/timeseriesv2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ func ReuseWriteRequestV2(req *PreallocWriteRequestV2) {
req.data = nil
}
req.Source = 0

for i := range req.Symbols {
req.Symbols[i] = ""
}
req.Symbols = req.Symbols[:0]
if req.Timeseries != nil {
ReuseSliceV2(req.Timeseries)
Expand Down
32 changes: 32 additions & 0 deletions pkg/cortexpb/timeseriesv2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,38 @@ func TestTimeseriesV2FromPool(t *testing.T) {
})
}

func TestReuseWriteRequestV2(t *testing.T) {
Copy link
Copy Markdown
Member

@CharlieTLe CharlieTLe Mar 29, 2026

Choose a reason for hiding this comment

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

Could this test flake since sync.Pool doesn't guarantee Get returns the same object that was just Put? If the GC runs between the ReuseWriteRequestV2 and PreallocWriteRequestV2FromPool calls, a fresh object would be returned and the backing array assertions would be skipped.

Would it be more robust to capture a reference to the backing array before the reuse call, so the clearing can be verified directly without depending on the pool?

// Capture backing array before reuse
backingArray := req.Symbols[:cap(req.Symbols)]

ReuseWriteRequestV2(req)

// Verify clearing directly on the backing array
for i, s := range backingArray[:3] {
    assert.Equalf(t, "", s, "symbol at index %d not cleared", i)
}

Copy link
Copy Markdown
Member Author

@SungJin1212 SungJin1212 Mar 30, 2026

Choose a reason for hiding this comment

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

I fixed it at the latest commit to check if the symbol table is reset without GC.
Thanks for the comment!

req := PreallocWriteRequestV2FromPool()

// Populate req with some data.
req.Source = RULE
req.Symbols = append(req.Symbols, "", "__name__", "test")

tsSlice := PreallocTimeseriesV2SliceFromPool()
tsSlice = append(tsSlice, PreallocTimeseriesV2{TimeSeriesV2: TimeseriesV2FromPool()})
req.Timeseries = tsSlice

// Capture backing array before reuse
symbolsBackingArray := req.Symbols[:cap(req.Symbols)]
require.Equal(t, "__name__", symbolsBackingArray[1])
require.Equal(t, "test", symbolsBackingArray[2])

// Put the request back into the pool
ReuseWriteRequestV2(req)

// Verify clearing directly on the backing array
for i, s := range symbolsBackingArray[:3] {
assert.Equalf(t, "", s, "symbol at index %d not cleared", i)
}

// Source is reset to default
assert.Equal(t, API, req.Source)
// The symbol length is properly reset to 0.
assert.Len(t, req.Symbols, 0)
// Timeseries slice is nil
assert.Nil(t, req.Timeseries)
}

func BenchmarkMarshallWriteRequestV2(b *testing.B) {
ts := PreallocTimeseriesV2SliceFromPool()

Expand Down
Loading