perf(chunksfts): rank the whole workspace in one FTS5 query - #265
Open
dvcdsys wants to merge 4 commits into
Open
perf(chunksfts): rank the whole workspace in one FTS5 query#265dvcdsys wants to merge 4 commits into
dvcdsys wants to merge 4 commits into
Conversation
Stage 2 of the search-perf plan, and the change stage 1's timings pointed at: BM25 was 78-80% of the workspace fan-out's work. The reason is structural. FTS5 drives the query: it evaluates MATCH over the WHOLE chunks_fts table — every project on the server — joins each hit to chunks_meta, and only then discards the rows belonging to other projects. So a per-project BM25 query costs about the same whichever project it names, and a 43-project workspace paid for the same global match 43 times. On top of that the 43 queries contended over one index: a repo's BM25 measured 326-542 ms standalone but up to 7,000 ms inside the fan-out. chunksfts.SearchProjects replaces them with one statement that ranks within each project via ROW_NUMBER() OVER (PARTITION BY project_path ORDER BY bm, rowid) and keeps each project's top rows. The IN list is batched at 500 paths to stay under SQLite's 999-variable ceiling, the same batch size the vector store already uses. In the handler the query runs in the fan-out's errgroup alongside the dense scans rather than before them, so nothing serialises; fusion moves out of the per-project goroutines because it now needs both sides, and it was under a millisecond across the whole fan-out anyway. Measured on the 45-repo fixture (1.9M chunks, 43-project workspace, 10 queries, medians). Both builds were run back to back against the same already-warm page cache — a process restart does not evict it — with one warm-up pass discarded each time, because a first comparison across a cold restart credited this change with twice the improvement it earned: phase develop stage 2 wall 10,235 4,650 2.2x fan-out 9,961 4,376 2.3x BM25 111,210 4,375 25.4x (summed over 43 -> one query) dense sum 15,166 11,136 1.4x dense max 2,318 1,123 2.1x The dense rows are the ones worth pausing on: nothing in the dense path changed. Removing 43 concurrent FTS queries gave the vector scans back the CPU and I/O they were contending for, which is worth 1.4x on the work and 2.1x on the project anyone actually waits for. BM25 is no longer the dominant term: at 4,375 ms it now sits level with the fan-out's own wall time, so the single FTS query IS the critical path. Whatever comes next should start there rather than from the old 78-80% figure. Correctness, on the fixture, 50 queries, full response captured per query: the BM25 signal is IDENTICAL in all 500 panel rows, and the project panel order is identical for all 50 queries. Dense scores wobble by <=0.0015 in a few percent of rows — but the same binary compared against ITSELF wobbles at least as much (30 rows vs 38), so that is a pre-existing property of the fixture, not this change. Its cause is not established; single-project search repeats bit-identically, and three consecutive workspace queries repeat bit-identically, so it correlates with machine load rather than with the query. Both orderings are (bm ASC, rowid ASC). The rowid is defensive rather than a fix: bm25 ties are the norm in a trigram index — 14 of 16 hits in the package's own test corpus share a score — and SQLite happens to return tied rows in rowid order for both the LIMIT and the window form today, so they agree without being told to. That is unspecified sorter behaviour, and naming the tiebreak makes the agreement a property of the queries instead of a coincidence. bm25_sum_ms and bm25_max_ms collapse into bm25_ms. The split existed to separate "work done" from "waited for" across N queries; with one query they are the same number, and keeping both would imply a fan-out that no longer happens. The blast radius grew and the tests say so: BM25 used to fail per project, and now one failing query costs every project its sparse signal at once. The fallback is the one a pre-FTS install already lives with — dense-only results, no failed_repos, no 500 — and TestWorkspaceSearch_SurvivesBM25Failure drops chunks_fts outright to prove it. Tests, each mutation-checked against the bug it describes: - SearchProjects matches SearchProject per project, same hits, same order, same scores, over four queries x two limits x four projects on a tie-heavy corpus; - the IN list does not prefix-match (project paths routinely share prefixes: "local:host:/x" vs "local:host:/x/y"); - the map survives the batch boundary (searchProjectsBatch + 7 projects); - BM25 hits stay in their own project end-to-end through the handler; - a total BM25 failure still returns dense results. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by clicking the fixture dashboard: "google authentication login
form" took 18.6 s, against a 4.6 s median. The slow-query log said the
whole 18.6 s was the single BM25 statement, so the timings from stage 1
pointed straight at it.
The cause was not the partitioning, it was what the CTE carried. Selecting
file_path, content and the rest inside `hits` makes SQLite materialise all
of it for EVERY matched row before ROW_NUMBER trims to perProject per
project — and the match set is the whole server's index, because that is
how FTS5 evaluates MATCH. That query matched 263,515 rows to return 2,300.
Ranking on (project_path, rowid, bm) alone and joining chunks_meta and
chunks_fts back for the survivors costs one rowid round-trip per returned
row and nothing per discarded row.
Measured with loadtests/bench/ftsab (the server's own driver, serial, so
the old shape's sum is not hidden by the fan-out's concurrency):
query match set 46 queries payload rank
in CTE first
google authentication login form 263,515 26,085 15,070 2,798
parse JWT token and validate sig. 623,913 27,587 29,566 5,492
rate limiter middleware 166,347 16,904 7,692 993
websocket upgrade handshake 21,049 3,505 1,011 156
graceful shutdown on SIGTERM 14,987 3,244 854 424
The JWT row is the one that matters: with a 624k-row match set the query
as shipped was SLOWER than the 46 per-project queries it replaced. The
gain scaled inversely with the match set — exactly backwards — and the
ten-query bench set hid it because the old shape ran concurrently in the
fan-out while these numbers are serial.
End to end on the fixture, same warm cache, 10 queries, medians:
phase develop prev commit this commit vs develop
wall 10,235 4,650 3,048 3.4x
BM25 111,210 4,375 2,782 40.0x
dense sum 15,166 11,136 11,649 1.3x
dense max 2,318 1,123 1,138 2.0x
And the query that started this: 18,623 ms -> 2,711 ms.
No test guards this. It is a property of the query plan, not of the
result, and the equivalence tests pass against both forms — they did, and
that is the point: correctness tests cannot see this class of bug. What
guards it is loadtests/bench/ftsab, which times all three shapes against
the real corpus, and the comment on the query saying why the obvious form
is wrong.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…invariant N1, the one worth doing: 6a47df7's commit message named loadtests/bench/ftsab as the guard against the payload-in-CTE regression returning. /loadtests/ is gitignored, so that guard exists on one machine and the reference is worse than none in a repo whose commit messages are written for the next agent. The in-repo guard asserts the query PLAN, because that is the only place this bug lives: every equivalence test in the package passes against the slow form too — verified, not assumed — since both forms return the same rows. FTS5 reports a rowid lookup as "0:=" and a MATCH scan as "0:M..."; the rank-first form does both, the payload-in-CTE form only scans. workspaceRankQuery is extracted so the test builds the string production builds rather than a copy free to drift. Its mutation check lives in the tree rather than in a shell history: TestExplainDistinguishesTheTwoQueryShapes builds the slow form and asserts the plan assertion REJECTS it. Without that, a change in how SQLite reports plans could quietly turn the guard into a tautology. Confirmed by mutation: moving the payload back into the CTE fails the plan test while the equivalence tests still pass. Also, and where the reasoning differs from the review: - F2, bm25_ms had no invariant left after sum/max collapsed. Added bm25_ms <= fanout_ms. Stated plainly in the comment is what it does NOT catch: a dropped assignment, since 0 <= fanout holds. "> 0" is not available because an in-memory corpus rounds to 0 ms, and a flaky guard is worse than an honest partial one. - F3, the batch loop hand-copied scanHit's eleven lines and lost the sign-flip comment. Both paths now share scanRankedHit, which takes an optional leading project_path. This mattered more than it looks: the equivalence test compares the two paths against EACH OTHER, so a mistake made symmetrically in both would have passed. - F4, the mutex around bm25ByProject guarded nothing — single writer, readers after g.Wait(). Dropped, with a comment saying why, because a lock that protects nothing reads like protection to whoever next needs those hits inside the fan-out. - F5, denseHits[i] is released as the fusion loop consumes it. Fusion used to free its inputs per goroutine; without this, every project's dense hits, BM25 hits and fused copies stay live at once. - F7, placeholders' n<=0 branch returned "NULL", which matches nothing and is indistinguishable from "nothing matched". Removed: IN () is a syntax error, which is loud and points at the wrong caller. - F9, the searchPhases header still said "the fan-out phases keep a SUM and a MAX" after this PR left only dense with that shape. F6/F8 — collapsing SearchProject into SearchProjects([]string{p}) — NOT done, deliberately. It would make TestSearchProjects_MatchesPerProjectQueries compare a function to itself, and that test is the only independent check that the partitioned ranking matches the known-good per-project one. The per-project BM25 signal feeds project candidacy, so a divergence re-ranks the projects panel with no error and no failed_repos. Structural agreement is worth less here than an oracle. SearchProject's doc comment now says it has no production caller, why it is kept, and not to delete it as unused — which is the real fix for F8. go test ./... green (46 packages), -race green on both changed packages, go vet and gofmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by chasing "why is the answer different each time". Not caused by this branch — main has it too — but it is the larger half of the answer, so it lands here rather than waiting. fuseRRF built its output slice by ranging over a map. Go randomises map iteration deliberately, and sort.SliceStable then preserved that randomness for every pair of chunks with equal RRF. Equal RRF is not a corner case: a chunk found only by dense at rank r and a chunk found only by BM25 at the same rank r score identically by construction, which happens in most queries. The symptom was invisible from the projects panel — project scores do not depend on chunk order — so the panel looked stable while the chunk list underneath it moved. On the fixture, the same query on the same process and binary returned a different chunk at rank 0 between consecutive calls. Sorting by (rrf desc, chunk key asc) gives a total order. Measured on the 43-project fixture, two full 50-query sweeps of one build against itself: chunk lists differing 25/50 -> 5/50 The five that remain are not ours. The provider returns a different vector for a byte-identical request often enough to matter: logging the exact request body alongside a checksum of the vector it produced, over two sweeps, 4 of 50 queries got two distinct vectors from identical bodies (sha of the marshalled request equal, sha of the float32 vector not). When a query drifts it drifts in ALL ten panel projects at once, which is the signature of the query vector moving rather than of any per-collection scan. dense_score shifts by <=0.002 and occasionally flips a rank. Nothing in cix can make that deterministic; a query-embedding cache keyed on the text would, and would cut provider spend too, but that is a separate change with its own trade-offs. The test asserts across 20 repeats, because with N tied entries a single run has a 1/N! chance of looking ordered by accident. It also pins that RRF still dominates the key: a chunk present in both lists outranks single-list chunks whatever its key sorts like. Mutation-checked — restoring the SliceStable-without-tiebreak form fails it on run 0. Co-Authored-By: Claude Opus 5 <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
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.
Stage 2 of the search-perf plan, and the change stage 1's timings pointed at.
Follows #263 (int8 scan) and #264 (per-phase timings).
Why BM25 was the whole problem
FTS5 drives the query. It evaluates
MATCHover the wholechunks_ftstable — every project on the server — joins each hit to
chunks_meta, and onlythen discards the rows belonging to other projects. A per-project BM25 query
therefore costs about the same whichever project it names, and a 43-project
workspace paid for the same global match 43 times.
On top of that the 43 queries contended over one index: a repo's BM25 measured
326–542 ms standalone but up to 7,000 ms inside the fan-out.
What changed
chunksfts.SearchProjectsreplaces the N queries with one statement that rankswithin each project and keeps each project's top rows:
The
INlist is batched at 500 paths to stay under SQLite's 999-variableceiling — the same batch size the vector store already uses. In the handler the
query runs inside the fan-out's errgroup alongside the dense scans rather than
before them, so nothing serialises. Fusion moves out of the per-project
goroutines because it now needs both sides; it was under a millisecond across
the whole fan-out.
bm25_sum_msandbm25_max_mscollapse intobm25_ms. The split existed toseparate "work done" from "waited for" across N queries; with one query they are
the same number, and keeping both would imply a fan-out that no longer happens.
Measured
45-repo fixture (1.9M chunks, voyage-code-3 @2048), 43-project workspace,
10 queries, medians. Both builds run back to back against the same already-warm
page cache — a process restart does not evict it — with one warm-up pass
discarded each time. This matters: a first comparison across a cold restart
credited the change with twice the improvement it earned.
The dense rows are worth pausing on: nothing in the dense path changed.
Removing 43 concurrent FTS queries gave the vector scans back the CPU and I/O
they were contending for — 1.4x on the work, 2.1x on the project anyone actually
waits for.
BM25 is no longer the dominant term — it is 19% of the fan-out's work at the
median, down from 78–80%. Dense is now the thing to look at.
Correctness on the fixture
50 queries, full workspace response captured per query (panel with both signals,
chunk list in order),
developvs this branch:shifts where that changes an RRF rank.
That last one is not this change. Running the same binary twice and
diffing it against itself produces at least as much wobble (30 differing rows
vs 38 across the build change). Its cause is not established and I did not chase
it: single-project search repeats bit-identically across three runs including
scores, and three consecutive identical workspace queries repeat
bit-identically, so it correlates with machine load rather than with the query
or the build. Worth its own look; it predates this PR.
Risks the plan flagged, and where they landed
identical in all 500 panel rows.
Not a problem at this corpus size; no outer bound added, so this is the thing
to watch on a much larger server.
the window degenerates to a sort over that project's hits. If a single-project
regression shows up,
SearchProjectis still there to fall back to.The blast radius grew, and the tests say so
BM25 used to fail per project. Now one failing query costs every project its
sparse signal at once. The fallback is the one a pre-FTS install already lives
with — dense-only results, no
failed_repos, no 500 — andTestWorkspaceSearch_SurvivesBM25Failuredropschunks_ftsoutright to proveit.
Tests
Each mutation-checked against the bug it describes:
SearchProjectsmatchesSearchProjectper project — same hits, same order,same scores — over 4 queries x 2 limits x 4 projects on a deliberately
tie-heavy corpus;
INlist does not prefix-match (project paths routinely share prefixes:local:host:/xvslocal:host:/x/y);searchProjectsBatch + 7projects);One honest gap: the
(bm, rowid)tiebreak is defensive, not test-provable.Ties are the norm in a trigram index — 14 of 16 hits in the package's own corpus
share a score — but SQLite currently returns tied rows in rowid order for both
the
LIMITand the window form, so dropping the explicit tiebreak still passes.Naming it makes the agreement a property of the queries rather than a
coincidence a future planner is free to break.
Verification
go test ./...green (46 packages),go test -racegreen on both changedpackages,
make openapi-checkin sync,go vetandgofmtclean.🤖 Generated with Claude Code
Second commit: a regression the tests could not see
Found by clicking the fixture dashboard by hand.
google authentication login formtook 18.6 s against a 4.6 s median, and stage 1's slow-query logattributed all of it to the single BM25 statement.
The partitioning was not the problem — what the CTE carried was. Selecting
file_path,contentand the rest insidehitsmakes SQLite materialise all ofit for every matched row before
ROW_NUMBERtrims. The match set is thewhole server's index, because that is how FTS5 evaluates
MATCH: that querymatched 263,515 rows in order to return 2,300.
Timed with
loadtests/bench/ftsabthrough the server's own driver, serially, sothe old shape's cost is not hidden by the fan-out's concurrency:
The JWT row is the one that matters: at a 624k-row match set the first commit
was slower than the 46 queries it replaced. The gain scaled inversely with the
match set — exactly backwards — and the ten-query bench set hid it because the
old shape runs concurrently in the fan-out while these numbers are serial.
The fix ranks on
(project_path, rowid, bm)alone and joinschunks_metaandchunks_ftsback for the survivors: one rowid round-trip per returned row,nothing per discarded row. The query that started this went 18,623 ms → 2,711 ms.
No test guards this. It is a property of the query plan, not of the result —
the equivalence tests pass against both forms, which is precisely the point:
correctness tests cannot see this class of bug. What guards it is
loadtests/bench/ftsab, which times all three shapes against the real corpus,and a comment on the query explaining why the obvious form is wrong.
It also revises the risk I listed above as "not a problem at this corpus size;
no outer bound added". Match-set size was the problem, and it is now paid for
per returned row rather than per matched row.
Third commit: review findings
N1 — the guard I named was not in the repo. The previous commit closed with
"what guards it is
loadtests/bench/ftsab", and/loadtests/is gitignored, sothat guard existed on one machine. It is now a test that asserts the query
plan: FTS5 reports a rowid lookup as
0:=and a MATCH scan as0:M…; therank-first form does both, the payload-in-CTE form only scans.
workspaceRankQueryis extracted so the test builds the string productionbuilds rather than a copy free to drift.
Its own mutation check lives in the tree rather than in my shell history:
TestExplainDistinguishesTheTwoQueryShapesbuilds the slow form and asserts theplan assertion rejects it — otherwise a change in how SQLite reports plans
could quietly turn the guard into a tautology. Confirmed by mutation: moving the
payload back into the CTE fails the plan test while the equivalence tests
still pass, which is the review's point made executable.
F2 —
bm25_mshad no invariant after sum/max collapsed. Addedbm25_ms <= fanout_ms, and the comment states what it does not catch: adropped assignment, since
0 <= fanoutholds.> 0isn't available because anin-memory corpus rounds to 0 ms, and a flaky guard is worse than an honest
partial one.
F3 — both paths now share
scanRankedHit. This mattered more than it looks:the equivalence test compares the two paths against each other, so a mistake
made symmetrically in both would have passed.
F4 — the mutex around
bm25ByProjectguarded nothing (single writer, readersafter
g.Wait()); dropped, with a comment, because a lock that protects nothingreads like protection to whoever next needs those hits inside the fan-out.
F5 —
denseHits[i]released as the fusion loop consumes it.F7 —
placeholders(0)returned"NULL", which matches nothing and isindistinguishable from "nothing matched". Removed;
IN ()is a syntax error,which is loud and points at the wrong caller.
F9 — stale
searchPhasesheader.F6/F8 — not done, deliberately
Collapsing
SearchProjectintoSearchProjects([]string{p})would makeTestSearchProjects_MatchesPerProjectQueriescompare a function to itself, andthat test is the only independent check that the partitioned ranking matches the
known-good per-project one. The per-project BM25 signal feeds project candidacy,
so a divergence re-ranks the projects panel with no error and no
failed_repos.Structural agreement is worth less here than an oracle.
SearchProject's doccomment now says it has no production caller, why it is kept, and not to delete
it as unused — which is the real fix for F8.