spine: expose in-progress merges for memory accounting - #852
Draft
antiguru wants to merge 1 commit into
Draft
Conversation
A consumer that accounts for a spine's memory footprint by walking `TraceReader::map_batches` undercounts it. For a layer mid-merge, that method presents the merge's two input batches and stops, but the layer holds a third allocation: the merger's partially assembled output. `Merger::new` sizes that output's containers with `BatchContainer::merge_capacity`, so the allocation appears in full the moment the merge begins and is invisible for the merge's whole duration. Add `Spine::map_mergers`, which applies a closure to the merger of each layer that is currently mid-merge. It is an inherent method rather than a `TraceReader` one because `TraceReader` has no `Merger` associated type, and its `Batch` is bounded by `BatchReader` rather than `Batch`, so the merger type is not nameable at that level. Reaching a merger is only useful if its contents can be inspected, so also add `OrdValMerger::result` and `OrdKeyMerger::result`, which borrow the storage being assembled, and `RcMerger::inner`, without which `map_mergers` on the default `Rc`-backed spines yields an opaque wrapper. All three additions are accessors. No existing signature, trait, or behavior changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
A consumer that accounts for a spine's memory footprint by walking
TraceReader::map_batchesundercounts it. For a layer mid-merge, that method presents the merge's two input batches and stops, but the layer holds a third allocation: the merger's partially assembled output.Merger::newsizes that output's containers withBatchContainer::merge_capacity, so the allocation appears in full the moment the merge begins, and stays invisible for the merge's whole duration.This adds three accessors so a caller can reach that storage.
Spine::map_mergersapplies a closure to the merger of each layer currently mid-merge. It is an inherent method rather than aTraceReaderone becauseTraceReaderhas noMergerassociated type, and itsBatchis bounded byBatchReaderrather thanBatch, so the merger type is not nameable at that level.OrdValMerger::resultandOrdKeyMerger::resultborrow the storage being assembled.OrdValBuilder::resultis already public, so this exposes nothing new in kind.RcMerger::innerborrows the wrapped merger. Without it,map_mergerson the defaultRc-backed spines yields an opaque wrapper and the new method is unusable.No existing signature, trait, or behavior changes. The whole diff is 44 added lines.
Motivation
Materialize reports per-arrangement heap size, capacity, and allocation counts as introspection relations and Prometheus metrics, computed by summing over
map_batches. Because in-progress merges are invisible, those numbers understate a merging arrangement.Measured on a one-million-row index across 87 observations, comparing what
map_batchesalone reports againstmap_batchesplusmap_mergers:The undercount grows with merge progress, from a median 1.07x while the merge output is under a quarter full to 1.91x once it is over three quarters full. That is the wrong shape: the accounting is accurate while a merge is barely started and wrong by nearly a factor of two once the output is nearly built, which is also when the input batches are still resident and the arrangement's real footprint peaks. At the peak observation, 30.3 MB were reported against 59.0 MB actually written.
Ratios can exceed 2x because a container sizes its merged reservation from the inputs' record counts rather than from the inputs' own allocations.
Notes for review
map_mergersdoc comment says so, since it is the one way this API is easy to misuse.OrdValMerger::staging(anUpdsBuilder) remains inaccessible. It holds one key/value pair's worth of times and diffs, bounded by the maximum multiplicity of a single pair, which is negligible against the output. Exposing it would need further accessors onUpdsBuilder, so it is deliberately left out.BatcherEvent(which already carries size, capacity, and allocation deltas) with a spine-level equivalent. That needs aheap_sizenotion onBatchContainerand an implementation for every container, and it duplicates caching that consumers already have. These accessors are the smaller change.Draft because it is paired with an unmerged Materialize change that consumes it.
🤖 Generated with Claude Code