Repository navigation
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
fionaliao
force-pushed
the
fl/mqe-binop-untracked-memory
branch
2 times, most recently
from
November 2, 2026 19:54
0757f05 to
c5bae88
Compare
computeOutputSeries called l.Bytes(buf) twice for each new output series and did not keep the result. For label sets larger than types.LabelBytesBufferSize, each call allocated a new buffer. Call Bytes once and assign the result back to buf, as the one-to-one operator already does. In a local benchmark of SeriesMetadata with 10k output series and labels over the buffer size, this allocated 47% fewer bytes, made 37% fewer allocations and took 35% less time. Small label sets did not change.
fionaliao
force-pushed
the
fl/mqe-binop-untracked-memory
branch
3 times, most recently
from
November 3, 2026 17:35
9bde44d to
0a37008
Compare
One-to-one and grouped binary operations key outputSeriesMap by a copy of each output series' labels. This copy was not included in the memory consumption estimate. For queries that produce many output series, such as a range topk on the "one" side of a group_left join, the copies are a large part of the peak memory use, so the query could exhaust memory before the per-query limit rejected it. Count the copies under a new BinaryOperationOutputSeries memory source while the map exists, and release them when computeOutputSeries returns.
fionaliao
force-pushed
the
fl/mqe-binop-untracked-memory
branch
from
November 3, 2026 17:38
0a37008 to
8ebe742
Compare
fionaliao
changed the title
fionaliao
marked this pull request as ready for review
November 3, 2026 17:53
56quarters
approved these changes
Nov 3, 2026
|
|
||
| types.SeriesMetadataSlicePool.Put(&outputSeries, memoryConsumptionTracker) | ||
| require.NoError(t, o.FinishedReading(ctx)) | ||
| o.Close() |
Contributor
There was a problem hiding this comment.
Should we assert that there are zero bytes in use after the operator is closed?
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
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.
GroupedVectorVectorBinaryOperation and OneToOneVectorVectorBinaryOperation both create temporary maps of output series, with a copy of the series labels being the key. These copied labels are unaccounted for in the memory consumption estimate. This causes the actual memory usage to be higher than the estimate, occasionally leading to OOMs that should be protected against.
This is particularly a problem for GroupedVectorVectorBinaryOperation, because its output series can be a cross join of the two sides. Each "many" side series is paired with every "one" side series in its group that is present at any point in the query time range. The output series map can then be much larger than either input, so the untracked label copies are a large part of the memory used.
This PR counts the copied labels in the estimate while the map exists, and releases them once the output series are returned.
It also makes GroupedVectorVectorBinaryOperation keep the buffer it encodes labels into, as OneToOneVectorVectorBinaryOperation already does. Before, the result of
Bytes()was discarded, so the buffer never grew past its initial 1 KB. Every label set larger than that allocated a new temporary buffer, twice for each output series.