Skip to content

Querier: remove redundant chunk data copy in ingester streaming select - #7746

Draft
sandy2008 wants to merge 4 commits into
cortexproject:masterfrom
sandy2008:fix/7732-remove-redundant-chunk-copy
Draft

Querier: remove redundant chunk data copy in ingester streaming select#7746
sandy2008 wants to merge 4 commits into
cortexproject:masterfrom
sandy2008:fix/7732-remove-redundant-chunk-copy

Conversation

@sandy2008

Copy link
Copy Markdown
Contributor

What this PR does:

Removes the detachChunksFromBuffer copy that #7670 (unreleased) added to the querier's ingester streaming-select path. The chunk copy is redundant: chunk data never aliases the gRPC receive buffer, because Recv() allocates a fresh QueryStreamResponse per message, TimeSeriesChunk.Unmarshal appends a zero Chunk value, and Chunk.Unmarshal's m.Data = append(m.Data[:0], ...) therefore starts from a nil slice and allocates a private backing array. The copy cost one allocation + memcpy per chunk and kept the original and the copy simultaneously reachable across the streamingSelect loop, raising peak heap — the opposite of #7670's goal.

The label copy (FromLabelAdaptersToLabelsWithCopy) is unchanged and still required: LabelAdapter.Unmarshal uses yoloString, so label names/values genuinely alias the receive buffer.

The issue asked maintainers to confirm no code path constructs client.Chunk values that alias a shared or pooled buffer before removing the copy. That verification is part of this PR:

  • Cortex registers a custom codec (pkg/cortexpb/codec.go, encoding.RegisterCodecV2) built on grpc-go's pooled mem.BufferSlice machinery — but pooling only affects where the wire bytes live. Decoding still dispatches (via the protobuf-v2 legacy shim) to the gogo-generated Unmarshal, which copies []byte fields unconditionally. QueryStreamResponse does not implement ReleasableMessage, so the codec never returns its buffer to a pool while decoded messages are live.
  • A repo-wide search (codec registrations/overrides, sync.Pools, vtprotobuf, unsafe, every client.Chunk{...} construction site) found no producer whose Data aliases shared memory on the read path.
  • Two tests pin the invariant so a future pooled or zero-copy decoder fails loudly: a direct gogo marshal→unmarshal round trip asserting chunk data does not overlap the wire buffer while a label value does (with a wire-buffer mutation cross-check that validates the test methodology itself), and the same invariant through the actually-registered codec with a payload above the gRPC buffer-pooling threshold (with a self-check that it stays above it).
  • The call-site comment documents the assumption the removal relies on (response messages are never reused across Recv calls) and that a detach copy must be reinstated if QueryStreamResponse ever becomes pooled.

The #7670 benchmark constructed its input by hand-slicing one shared buffer into all chunks — a shape a real gRPC unmarshal never produces, which is why the copy had appeared profitable. It is rebuilt with independently allocated chunk data; on that input, dropping the copy saves ~107 KB and 200 allocations per 100-series response batch (1002 vs 1202 allocs/op; 168 KB vs 275 KB/op — allocation numbers are the meaningful signal, time-per-op is noise-dominated at this size).

Also noticed while verifying, deliberately not touched here: detachSeriesFromBuffer in pkg/querier/blocks_store_queryable.go (added by #7519, store-gateway path, vendored Thanos storepb types) has a similar shape and may warrant the same analysis separately.

Which issue(s) this PR fixes:
Fixes #7732

Checklist

  • Tests updated
  • Documentation added (n/a — no config or flag changes)
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX] (the unreleased Detach ingester series from gRPC buffers to reduce heap #7670 entry is amended to describe the final behavior)
  • docs/configuration/v1-guarantees.md updated if this PR introduces experimental flags (n/a)

Per the Generative AI Contribution Policy: this change was developed with substantial AI assistance (multiple independent AI implementations reconciled and then adversarially cross-reviewed, under my direction). I have reviewed and validated all of it and take responsibility for its correctness.

detachChunksFromBuffer copied every chunk's data on the querier's
ingester-read path to detach it from the gRPC receive buffer, but the
chunk data never aliases that buffer in the first place: Recv() allocates
a fresh QueryStreamResponse per message, TimeSeriesChunk.Unmarshal appends
a zero Chunk value, and Chunk.Unmarshal's append therefore starts from a
nil slice and allocates a private backing array. The copy cost an extra
allocation and memcpy per chunk and kept both the original and the copy
reachable for the duration of streamingSelect, raising peak heap.

Labels are different: LabelAdapter.Unmarshal uses yoloString, so label
names and values do alias the receive buffer, and the
FromLabelAdaptersToLabelsWithCopy call is kept. The call-site comment now
documents both facts, and the assumption the removal relies on: response
messages are never reused across Recv calls. If QueryStreamResponse ever
becomes pooled, a detach copy must be reinstated.

Two tests pin the non-aliasing invariant, through a direct gogo round trip
(with a wire-buffer mutation cross-check) and through the registered
cortexCodec with a payload large enough to exceed the gRPC buffer-pooling
threshold, so a future pooled or zero-copy decoder fails loudly. The
original cortexproject#7670 benchmark, which constructed chunk data as sub-slices of
one shared buffer (a shape a real gRPC unmarshal never produces), is
rebuilt with independently allocated chunk data: dropping the copy saves
~107 KB and 200 allocations per 100-series response batch.

Fixes cortexproject#7732

Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
…romQLFuncsWithPrometheus / compactor unit test)

Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
…ce, FailedWithHaltError)

Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

detachChunksFromBuffer copies chunk data that Unmarshal already allocated separately (#7670)

1 participant