compute: walk fast-path index peeks off the serving worker - #38429
Draft
antiguru wants to merge 3 commits into
Draft
compute: walk fast-path index peeks off the serving worker#38429antiguru wants to merge 3 commits into
antiguru wants to merge 3 commits into
Conversation
Takes an owned, `Send` snapshot of a peek's cursor and walks it on a blocking task
instead of inline on the timely worker that received it, so a long scan no longer
delays the peeks queued behind it. The snapshot owns the `Arc` batches its cursor
covers, which is what makes it `Send`: a worker reads its own traces through an
`Rc`-based reader, so borrowing from one, or owning `Rc` batches, would not cross a
thread. The walk is generic over the cursor source, so any owned snapshot feeds it.
Bounded by `index_peek_offload_max_inflight`, because each in-flight walk retains the
batches its cursor covers. `mz_index_peek_walks_total{substrate}` exists so that "the
offload changed nothing" and "the offload never engaged" are distinguishable, which
cost a round of staging measurement before it did. An offloaded walk can divert to the
peek response stash partway through, without which the feature is unreachable in a
production configuration, since production runs the stash on.
The errs scan and the ok-iterator drain move out of `collect_finished_data` and
`collect_ok_finished_data` into `scan_errs_for_error` and `drain_ok_iterator`, which
are generic over the trace so the inline walk and the offloaded walk share them.
PARKED. This is one of four candidates for the same defect, and the experiment that
chooses between them has not run. Cooperative peek yielding (MaterializeInc#38040) is a second, and
placement and preemption are independent axes, so an off-worker walk that yields is an
untried fourth. Measurement already found this mechanism reproducibly worse than doing
nothing when a peek queues behind a long operator activation. The decision rule is
registered in CPU-217: if yielding alone matches this on E1, E11 and E8b, this should
be deleted rather than merged.
It exists as a branch so the experiment can deploy it, not because it is on its way in.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
antiguru
marked this pull request as draft
August 24, 2026 13:44
The walk counter had two values, so a peek that wanted to offload and ran inline at the in-flight cap was indistinguishable from one that ran inline because the flag was off. Those need different responses: the first is `index_peek_offload_max_inflight` being too low for the workload, the second is configuration. `substrate="capped"` is the third value, and the three partition every walk. Cap saturation is self-reinforcing, which is why it needs its own counter. Slots return when walks finish, so a worker whose walks run long accumulates in-flight walks, reaches the cap, and falls back to the inline walk that blocks its step loop, which makes it accumulate faster still. The worker that most needs the offload is the first to lose it, and without this counter that reads as the offload simply not helping. `WalkPlacement` replaces the `Option<InFlightOffload>` the dispatch used, so the inline arm knows which of the two reasons put it there. Also correct the `INDEX_PEEK_OFFLOAD_MAX_INFLIGHT` documentation, which claimed an in-flight walk holds the trace back from compacting. It does not: the walk owns `Arc` batches and the dispatching path drops its trace handle before the walk starts, so the cost is retained memory and a blocking-pool thread. The doc now also says the count is per worker, so a replica's bound is `workers * this`, and that a walk diverting to the stash holds its slot across the persist upload rather than just the cursor walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`TraceReader::cursor` returns its batches by value, and since the Arc-backed production spines those batches are `Arc`s, so the pair it hands back already owns everything it reads and is `Send` as it stands. `local_snapshot` existed to produce a property the cursor already had. Deleting it removes the module, its `LocalSnapshot` and `SnapshotError` types, and the unreachable soft-panic arm that only existed to handle a snapshot failure that `cursor` cannot report. The compaction gate the snapshot duplicated is still applied: `snapshot_for_offload` checks the trace bundle's compaction frontier against the peek time before taking any cursor, as the inline walk does. The `Send` assertion over `PeekResultIterator` moves to that type's own module, since it is a property of the cursor rather than of any wrapper, and it is what lets the walk leave the worker that owns the trace. Also restore `seek_fulfillment`'s doc comment, which a merge had concatenated onto the front of `snapshot_for_offload`. 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
1 participant
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.
Status: parked, not seeking review
This exists so the experiment that decides its fate can deploy it. It is not on its way in, and it should not be reviewed toward merge. Tracked as CPU-217.
There are four candidates for one defect, and the grid is two independent axes rather than one:
The decision rule, registered before the experiment runs: if yielding alone matches this on E1, E11 and E8b, this should be deleted rather than merged. If it wins on E8b alone, its case narrows to the unattributed swap-walk duration and nothing else.
One measurement already counts against it. E12 found it reproducibly worse than doing nothing when a peek queues behind a long operator activation, because dispatch happens in
process_peeksafterstep_or_parkreturns and retirement costs another step. That cost is paid before the walk starts, so giving the walk yield points cannot recover it.What it does
Takes an owned,
Sendsnapshot of a peek's cursor and walks it on a blocking task instead of inline on the timely worker that received it, so a long scan no longer delays the peeks queued behind it.The snapshot owns the
Arcbatches its cursor covers, which is what makes itSend: a worker reads its own traces through anRc-based reader, so neither borrowing from one nor owningRcbatches would cross a thread. That dependency is satisfied by the Arc-backed production spines merged in #38396. The walk is generic over the cursor source, so any owned snapshot feeds it.Bounded by
index_peek_offload_max_inflight, since each in-flight walk retains the batches its cursor covers, a memory bound rather than a concurrency knob.mz_index_peek_walks_total{substrate}exists so that "the offload changed nothing" and "the offload never engaged" are distinguishable, which cost a round of staging measurement before it existed. An offloaded walk can also divert to the peek response stash partway through, without which the feature is unreachable in a production configuration, because production runs the stash on.The errs scan and the ok-iterator drain move out of
collect_finished_dataandcollect_ok_finished_dataintoscan_errs_for_erroranddrain_ok_iterator, generic over the trace so the inline walk and the offloaded walk share them.Relation to the two-runtime work
Extracted from #38242, which carried this on top of the interactive runtime. Nothing here depends on that branch, and the interactive runtime does not need any of it: E2 showed the peek-tail win belongs to the walk substrate rather than to a second runtime. This branch is based directly on main and stacks on nothing.
The flag naming discussed on #38239 (a named substrate rather than a boolean, since placement and preemption are independent) is not implemented here. It is specified only, and deliberately not built while the mechanism itself is undecided.
Verification
cargo check --workspace --all-targetsclean,bin/lintandbin/fmtclean, no new clippy warnings.cargo test -p mz-compute --lib37/37, includingoffloaded_walk_matches_the_inline_walk, which asserts the offloaded walk returns exactly what the inline walk returns.🤖 Generated with Claude Code