Skip to content

compute: borrow each correction chunk once per merge, not once per update - #38649

Closed
antiguru wants to merge 3 commits into
MaterializeInc:mainfrom
antiguru:moritz/correction-borrow-hoist
Closed

antiguru wants to merge 3 commits into
MaterializeInc:mainfrom
antiguru:moritz/correction-borrow-hoist

Conversation

@antiguru

@antiguru antiguru commented Sep 4, 2026

Copy link
Copy Markdown
Member

Stacked on #38648. Review the last commit; the diff shows #38647 and #38648 as well until they merge.

Every element access in CorrectionV2 re-decoded its chunk. Chunk::index was self.column().borrow().get(i), and Column::borrow on a serialized column rebuilds the struct-of-arrays view from the serialized header, so a k-way merge paid O(log k) header decodes per output update: 42.7% of an insert profile, ahead of the merge itself at 12.2%.

Borrowing is now hoisted to one view per chunk, or per chunk pair in the two-way merge, whose inner loop runs until either cursor crosses a chunk boundary. Cursor gained chunk_handle, reads_from, and get_with for callers that hold a view across cursor steps; get and index remain the documented single-access forms. Chain iteration, chunk splitting, and the time search index a hoisted view likewise.

Holding a view inside the iterator that updates_before hands across the persist writer's await needs the view to be Send, which is a new bound on the container. The container bounds move into a DataContainer helper trait so Data stays readable.

Insert improves 1.6x on a 4.19M-update append workload, and 2.0x under a memory cap, where each redundant decode also faulted the header page back in.

🤖 Generated with Claude Code

antiguru and others added 3 commits September 14, 2026 22:21
`compute_correction_v2_chunk_size` sizes `CorrectionV2`'s staging area, not
its chunks: chunk bodies are fixed at the columnar ship size. The staging size
sets how many updates accumulate before a chain is minted, and every chain
minted is a chain some later read has to merge. Raising the default from 8 KiB
to 2 MiB, one chunk body's worth, takes insert on a 4.19M-update append
workload from 3443 ms to 1860 ms at an unchanged peak RSS. The curve is
monotone across the range, so the default is bounded only by the per-sink,
per-worker cost of the staging vector, and the `correction` bench gains a
`bench_chunk_size` sweep to show it. The flag's description now says what it
sizes.

The stage counted its fill in tuples of `size_of::<(D, Timestamp, Diff)>()`,
which is 40 bytes for a `Row` however wide the row is, so a byte-sized flag
bounded neither the staging memory nor the reported size once rows spill out of
line. A new `DataBytes` trait on the buffer's data types reports each datum's
bytes with the heap included, the stage ships when the staged bytes reach the
flag, and its size metrics carry the same figure. `Row` answers with
`byte_len`, `DataflowErrorSer` with its serialized length.

The staging vector was allocated eagerly at the configured capacity and
re-allocated at that capacity on every flush. At 2 MiB per sink and worker that
is no longer free, so it grows on demand instead and gives its allocation back
on flush.

The mzcompose and parallel-workload flag tables follow the new default, since CI
runs the value listed there rather than the compiled default.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
`CorrectionV2::update_metrics` walked every chain and every chunk on every
operation, from four call sites including insert, so its cost grew with the
buffer: 17% of insert at 16.8M updates and rising. An `Accounting` handle,
shared by the buffer and by every structure that holds its chains, now carries
running record, size, and allocation totals, so a metrics update reads three
counters.

The totals hook the discipline the introspection logging already kept: a chain
is announced once when the buffer takes it and retired once before it is dropped
or consumed, so the totals and the logging cannot drift apart. Each chunk caches
its serialized size at construction, and each chain maintains the sum as chunks
are pushed, so no accounting step pages a chunk in.

The reported size is the logical serialized size of the bodies, resident or not.
The previous walk reported resident bytes only, which depended on what the pager
had done with each chunk since. Residency belongs to the pager, which changes it
without telling the buffer. Capacity is reported equal to size, since chunk
bodies are exactly sized when minted, and allocations count one per chunk plus
the staging vector while it holds one.

`metrics_totals_match_walk` checks the totals against a walk after inserts, a
partial drain, a since advance, and a consolidation.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
…date

Every element access in `CorrectionV2` re-decoded its chunk. `Chunk::index` was
`self.column().borrow().get(i)`, and `Column::borrow` on a serialized column
rebuilds the struct-of-arrays view from the serialized header, so a k-way merge
paid O(log k) header decodes per output update: 42.7% of an insert profile,
ahead of the merge itself at 12.2%.

Borrowing is now hoisted to one view per chunk, or per chunk pair in the two-way
merge, whose inner loop runs until either cursor crosses a chunk boundary.
`Cursor` gained `chunk_handle`, `reads_from`, and `get_with` for callers that
hold a view across cursor steps. `get` and `index` remain the documented
single-access forms. Chain iteration, chunk splitting, and the time search
index a hoisted view likewise.

Holding a view inside the iterator that `updates_before` hands across the
persist writer's `await` needs the view to be `Send`, which is a new bound on
the container. The container bounds move into a `DataContainer` helper trait
so `Data` stays readable.

Insert improves 1.6x on a 4.19M-update append workload, and 2.0x under a memory
cap, where each redundant decode also faulted the header page back in.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
@antiguru

Copy link
Copy Markdown
Member Author

Closed in favor of #38896, the same branch as a native stacked PR in this repository (stack #38898). Fork PRs cannot join GitHub stacks.

🤖 Posted by Claude Code

antiguru added a commit that referenced this pull request Sep 30, 2026
…date (#38896)

Every element access in `CorrectionV2` re-decoded its chunk.
`Chunk::index` was `self.column().borrow().get(i)`, and `Column::borrow`
on a serialized column rebuilds the struct-of-arrays view from the
serialized header, so a k-way merge paid O(log k) header decodes per
output update: 42.7% of an insert profile, ahead of the merge itself at
12.2%.

Borrowing is now hoisted to one view per chunk, or per chunk pair in the
two-way merge, whose inner loop runs until either cursor crosses a chunk
boundary. The k-way merge that reads over three or more chains works in
rounds the same way: each round borrows the current chunk of every input
and heaps their updates, and ends when an input crosses a chunk
boundary. `merge_many_consolidates_across_chunks` covers it. `Cursor`
gained `chunk_handle`, `reads_from`, and `get_with` for callers that
hold a view across cursor steps; `get` and `index` remain the documented
single-access forms. Chain iteration, chunk splitting, and the time
search index a hoisted view likewise.

Holding a view inside the iterator that `updates_before` hands across
the persist writer's `await` needs the view to be `Send`, which is a new
bound on the container. The container bounds move into a `DataContainer`
helper trait so `Data` stays readable.

Insert improves 1.6x on a 4.19M-update append workload, and 2.0x under a
memory cap, where each redundant decode also faulted the header page
back in. The new `column_borrow` bench in `mz-timely-util` measures that
decode: per-element borrows of a serialized column against one hoisted
borrow and a typed column.

Supersedes the fork PR #38649.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5.1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant