compute: offload the fast-path index-peek walk off the maintenance worker#37762
Closed
antiguru wants to merge 4 commits into
Closed
compute: offload the fast-path index-peek walk off the maintenance worker#37762antiguru wants to merge 4 commits into
antiguru wants to merge 4 commits into
Conversation
Stacked-PR roadmap for isolating reads from maintenance. Near-term slices (primitive fixes P1/P2, Stage 1 peek offload) planned to executable granularity; Stage 2 to PR boundary + DoD. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add LocalSnapshot<Tr>/snapshot_local: an owned CursorList over a local trace's Arc-backed batches taken via cursor_through, plus PeekResultIterator::new_over_snapshot to build a peek-result iterator over it. Compile-time assert_impl_all! proves both are Send, laying the groundwork for moving a fast-path index peek's cursor walk off the maintenance worker's thread. Also amends the stale !Send note in peek_stash.rs: with batches now Arc-backed, an owned snapshot cursor is in fact Send, so the existing channel-based hand-off to the async upload task is no longer forced by that constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move a fast-path index peek's cursor walk off the maintenance timely worker onto a spawned task, mirroring the existing persist-peek offload (oneshot + activator + abort-on-drop). Task 1a's owned, Send LocalSnapshot makes this possible: the worker takes a cheap snapshot of the oks/errs traces at their current upper, then a task builds a PeekResultIterator over it and drains the response. Gated by enable_index_peek_offload (default off). Peeks that are eligible for (and have) the peek response stash keep the synchronous path, since the stash's size-based diversion is decided mid-walk and isn't replicated in the offloaded path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`index_peek_total_seconds` was observed twice for offloaded fast-path index peeks: once in `process_peek` before dispatch, and again in the `IndexOffload` arm once the offloaded walk completes. Guard the pre-dispatch observation to skip the offloaded case, leaving the offload arm's task-duration observation as the single source for offloaded peeks. The synchronous path's observation is unchanged. Also corrects two comments left stale after the offload wiring landed: `local_snapshot`'s module doc claimed nothing in the module was called from the peek path yet, and `new_over_snapshot` still carried a "not called yet" comment plus an unneeded `#[allow(dead_code)]`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Superseded by #37770, which includes this Stage 1 work plus the full Stage 2 second-runtime implementation in one draft. Closing this in favor of that one. |
antiguru
changed the title
Member
Author
|
Closing. The peek offload this PR proposed turned out to buy nothing for its stated goal: the local trace is |
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.
Move the fast-path index-peek cursor walk off the maintenance timely worker onto an async task (the pattern persist peeks already use), so the walk no longer occupies the worker and the worker returns to maintenance without paying it. Standalone improvement to a single-runtime replica; orthogonal to (and reviewable independently of) the two-runtime work in #37770, which builds on this.
An owned
Sendpoint-in-time snapshot over the Arc-backed batches (LocalSnapshot), plus the flag-gated offload of the walk mirroring the persist-peek offload. Flagenable_index_peek_offload, default off. Point-lookup latency is unchanged (correctly); the win is that a large-result walk no longer runs on the worker thread.Scope, stated precisely: this removes the walk from the worker's critical path. It does NOT let a peek be serviced while the worker is busy. The snapshot is still taken inside
process_peeks, which runs only afterstep_or_parkreturns, so a peek against an index whose worker is mid-way through a long maintenance operator step still waits for that step to finish before it is initiated. Decoupling read initiation from maintenance busyness needs the interactive runtime (#37770), which reads Arc-published handles off the maintenance worker's loop entirely. The snapshot cannot be taken off-worker here because it reads the localRcTraceAgent.Stacks on the Arc-batches branch (#37743), so the base is that branch, not
main.Note: offload currently fires only for peeks not eligible for peek-response stashing (so under prod defaults,
ORDER BYpeeks); broadening to the common streamableSELECTneeds the offloaded path to handle mid-walk stash diversion (follow-up).🤖 Generated with Claude Code
https://claude.ai/code/session_01DAfFVVJ8BwahdaAHkRtbf6