spec: perf: async-find query-refinement fast-path#12192
Conversation
Co-Authored-By: Varoon Kodithala <116049637+vkodithala@users.noreply.github.com>
|
@oz-for-oss[bot] I'm starting a first review of this spec-only pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds product and technical specs for an async-find query-refinement fast path that reuses completed broader-query results to scan only candidate blocks/views. The specs cover the main behavior, validation plan, risks, and non-goals, and the security pass found no additional design-level security concerns for this local terminal-find optimization.
Concerns
- The technical refinement predicate omits selected-block scope equality even though the product spec says selected-block scope changes are not refinements; scope changes plus a query extension could reuse the wrong candidate set and miss matches.
- The technical spec does not account for the current
start_findcancellation ordering before checking prior status; the proposedstatus == Completegate needs a pre-cancel snapshot or moved cancellation, or the fast path can be skipped every time. - Focus preservation should define surviving occurrence identity by block/grid and position rather than exact old span equality, since a strict refinement changes the match span.
Verdict
Found: 0 critical, 2 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ## Proposed changes | ||
|
|
||
| ### 1. Replace the full refinement restart with a candidate-block restart | ||
| Keep the existing refinement predicate in `start_find`: regex disabled, same case-sensitivity, and `new_query` is a strict string prefix extension of the old query. Inside `filter_results_for_refinement`, build the new `AsyncFindConfig` first and return through `clear_results` for empty/whitespace queries exactly as today. |
There was a problem hiding this comment.
blocks_to_include or force a normal search on scope changes; otherwise a selected-block scope change plus a strict query extension can intersect against the old candidate set and miss matches, contradicting the product spec's non-refinement behavior.
| ### 1. Replace the full refinement restart with a candidate-block restart | ||
| Keep the existing refinement predicate in `start_find`: regex disabled, same case-sensitivity, and `new_query` is a strict string prefix extension of the old query. Inside `filter_results_for_refinement`, build the new `AsyncFindConfig` first and return through `clear_results` for empty/whitespace queries exactly as today. | ||
|
|
||
| Only take the candidate fast path when the previous broader run has reached `AsyncFindStatus::Complete`. If the broader run is still `Scanning` or has been cancelled back to `Idle`, fall back to the current full refined async search path so the refined query cannot miss blocks that the broader query had not scanned yet. |
There was a problem hiding this comment.
start_find calling cancel_current_find() before filter_results_for_refinement; require moving cancellation after the pre-cancel status/candidate snapshot or passing the prior status in, or the status == Complete gate will see Idle and the fast path will never run.
|
|
||
| 1. Snapshot the old focused match identity before clearing results. | ||
| 2. After each result batch, existing auto-focus/clamp behavior remains valid. | ||
| 3. After scan completion, if the old focused span survived, set `focused_match_index` to its new index and update the cache. |
There was a problem hiding this comment.
💡 [SUGGESTION] Define focus preservation by stable occurrence identity such as block/grid plus start position or containment, not exact old span survival; refining foo to foobar changes the end position even when the same visible occurrence remains.
1 participant
Summary
filter_results_for_refinement: fast-path only from completed prior scans, enqueue only previously result-bearing blocks/views, and complete immediately for completed zero-match prior queries.Validation
Related issue: #12037