Skip to content

compute: make the correction buffer's metrics O(1) - #38895

Merged
antiguru merged 1 commit into
moritz/correction-chunk-sizefrom
moritz/correction-metrics-o1
Sep 30, 2026
Merged

antiguru merged 1 commit into
moritz/correction-chunk-sizefrom
moritz/correction-metrics-o1

Conversation

@antiguru

@antiguru antiguru commented Sep 16, 2026 •

Copy link
Copy Markdown
Member

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, 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. 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, so mz_arrangement_sizes keeps its capacity and allocations columns for correction buffers.

metrics_totals_match_walk checks the totals against a walk after inserts, a partial drain, a since advance, and a consolidation. logged_chains_balance_on_drop checks that every chain announced to introspection, the stage's included, is retired when the buffer drops.

Supersedes the fork PR #38648.

🤖 Generated with Claude Code

@antiguru
antiguru added this pull request to stack #38898 September 16, 2026 17:59
@antiguru
antiguru marked this pull request as ready for review September 16, 2026 18:00
@antiguru
antiguru requested a review from a team as a code owner September 16, 2026 18:00
@antiguru antiguru changed the title moritz/correction metrics o1 compute: make the correction buffer's metrics O(1) Sep 16, 2026
@antiguru
antiguru requested a review from DAlperin September 16, 2026 21:08
@antiguru
antiguru force-pushed the moritz/correction-metrics-o1 branch from 3347975 to 001e6cb Compare September 17, 2026 13:54
@antiguru
antiguru requested a review from a team as a code owner September 23, 2026 13:23
@antiguru
antiguru force-pushed the moritz/correction-metrics-o1 branch from 001e6cb to a97c70e Compare September 23, 2026 13:23

@antiguru antiguru left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Self-review.

  • Bug: Stage::drop now goes through stage_diff(-len, ..). That logs chain_created(0) followed by chain_dropped(len), and when len == 0 it logs nothing. Either way the chain_created(0) from stage_created is never balanced, so net_batches stays one high for every dropped stage. CorrectionLogger::drop retracts the leftover at teardown, which is why nothing shows. Previously the drop logged chain_dropped(len). It needs a stage_dropped that restores that event and subtracts the totals.
  • Nit: chain_dropped uses an unchecked fetch_sub, while add_signed asserts on underflow. Both should use the same checked path.

🤖 Posted by Claude Code

@antiguru
antiguru force-pushed the moritz/correction-metrics-o1 branch from a97c70e to 4c1f1be Compare September 30, 2026 13:12
@antiguru

Copy link
Copy Markdown
Member Author

Addressed. Stage::drop now retires through stage_dropped, which logs chain_dropped(len) again, and logged_chains_balance_on_drop covers both an empty and a non-empty stage. The test fails against the previous drop with (1, 0) != (0, 0). Every subtraction now goes through sub_checked.

🤖 Posted by Claude Code

@antiguru
antiguru force-pushed the moritz/correction-metrics-o1 branch from 4c1f1be to 6332981 Compare September 30, 2026 15:44

@DAlperin DAlperin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks

`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.
`logged_chains_balance_on_drop` checks that every chain announced to
introspection, the stage's included, is retired when the buffer drops.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
@antiguru
antiguru force-pushed the moritz/correction-metrics-o1 branch from 6332981 to b4d3de1 Compare September 30, 2026 18:07
@antiguru
antiguru merged commit 3997e33 into main Sep 30, 2026
83 checks passed
@antiguru
antiguru deleted the moritz/correction-metrics-o1 branch September 30, 2026 19:45
antiguru added a commit that referenced this pull request Sep 30, 2026
`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 3503 ms to 1897 ms, with drain and
peak RSS unchanged. These are medians of three `correction_mem` runs
with the rest of the stack applied and no pool installed. The curve is
monotone across the range (the new `bench_chunk_size` sweep in the
`correction` bench shows it), so the default is bounded only by the
per-sink, per-worker cost of the staging vector.

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 (`Row` via
`byte_len`, `DataflowErrorSer` via its serialized length), the stage
ships when the staged bytes reach the flag, and its size metrics carry
the same figure.

`stage_ships_by_bytes` checks that rows of two widths ship after the
number of updates their bytes predict.

That 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 now grows on demand 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.
The flag's description now says what it sizes.

First of four, split out of #38645: this, the O(1) metrics (#38895), the
per-chunk borrow hoist (#38896), and the pool substrate (#38897) land in
that order. Supersedes the fork PR #38647.

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

Co-authored-by: Claude Fable 5.1 <[email protected]>
@linear-code

linear-code Bot commented Oct 1, 2026

Copy link
Copy Markdown

CPU-279

@bosconi bosconi mentioned this pull request Oct 2, 2026
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.

2 participants