fix(tests): isolate selection cost from dot-product cost in VectorSearcher benchmark - #33
Conversation
…rcher benchmark Search_At8735By1024RealisticScale_CompletesWithinAGenerousBound was flaky (issue #32): it timed the whole Search pipeline (dot products + heap selection) against a reference that recomputed the same dot products plus a full sort. Both paths pay the identical, dominant dot-product cost, which should cancel out of the ratio, but on one CI run it did not: the ratio spiked to 2.112 against a 1.5 ceiling, then passed on an identical re-run. Split VectorSearcher.Search into two internal steps, ComputeScores and SelectTopKFromScores, so the benchmark can time only the selection strategies against a fixed, precomputed scores array instead of the fused pipeline. This removes the shared dot-product cost from the measured window entirely, so the ratio reflects what the assertion has always claimed to compare. Reusing the real SelectTopKFromScores (via InternalsVisibleTo) rather than duplicating the heap logic in test code avoids re-deriving the tie-break rule that VectorSearcherTests already documents as easy to get subtly wrong. With the shared cost gone, local measurement clusters tightly around 0.02-0.03 across 60 runs (solo and under the suite's default parallelism), so the ceiling tightens from 1.5 to 1.0 while gaining roughly 30x headroom over the observed value.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The split moved a cost onto the production path that the PR did not state: the previous fused loop offered each score to the heap as it was computed and never held more than topK entries, while materialising all scores first makes every search allocate a float[Count]. Memory per search is therefore O(N), not O(topK). At measured scale that is ~35 KB of Gen0 garbage per query against ~190 ms spent waiting on the embedding call, so it is immaterial here -- but it is a real change, not a pure refactor, and it stops being immaterial on a much larger index. The remedy at that point is named in the comment: refuse the split for the production path and keep it only for the benchmark.
|
Дописал одно, чего в PR не хватало: разделение меняет профиль аллокаций боевого пути, и это не отражено ни в описании, ни в комментариях. Прежний слитый цикл считал очко и тут же предлагал его куче — в памяти жило не больше На нашем масштабе это 8735 float, примерно 35 КБ мусора Gen0 на запрос, против ~190 мс ожидания эмбеддинга и ~1,6 мс собственно поиска. Незначимо — но это вывод, который должен делать читатель, а не автор за него. На индексе на порядок-два больше значимость появится, и в комментарии теперь названо лекарство: вернуть слияние для боевого пути, оставив разделение только бенчмарку. Сам разбор причины флака — хороший. Гипотеза «разные реализации дот-продукта» проверена и отвергнута, а решающая улика найдена в самих числах упавшего прогона: Результат по разбросу говорит сам за себя: ratio 0,019–0,027 на 60 прогонах против прежних 0,6–0,9, порог ужесточён с 1,5 до 1,0. |
1 participant
Summary
Search_At8735By1024RealisticScale_CompletesWithinAGenerousBoundwas flaky (Flaky: VectorSearcherTests realistic-scale ratio assertion fails intermittently on ubuntu #32): it timed the wholeSearchpipeline (dot products + heap selection) against a reference that recomputed the same ~8735 x 1024 dot products plus a full sort. Both paths pay the identical, dominant dot-product cost, which should cancel out of the ratio — but on one CI run it did not (ratio 2.112 against a 1.5 ceiling), then passed on an identical re-run.VectorSearcher.Searchinto two internal steps,ComputeScoresandSelectTopKFromScores, so the benchmark can time only the selection strategies against a fixed, precomputed scores array instead of the fused pipeline. This removes the shared dot-product cost from the measured window entirely, so the ratio reflects what the assertion has always claimed to compare — bounded-heap selection vs. a full sort, not "score + select" vs. "score + select".SelectTopKFromScores(via a new, narrowly-scopedInternalsVisibleTo) rather than duplicating the heap logic in test code, since a hand-rolled copy risks silently drifting from the real tie-break rule (already the subject of a documented regression in this same test file).Root cause
Both
Searchand the reference full sort call the identicalTensorPrimitives.Dot— ruling out "different dot-product implementations." Code inspection found no allocation or hardware-branch asymmetry that would explain a 2x+ slowdown specific to the heap path. The most defensible explanation, given the CI failure was on an otherwise-identical re-run, is generic environment noise (GC/scheduler contention from xUnit's default parallel-by-collection execution) landing inside the measured window — made worse by the old test measuring "shared dot products + selection" instead of selection alone, so a few milliseconds of unavoidable shared cost diluted (and occasionally let noise dominate) the very small selection-cost signal the assertion cares about.Test plan
dotnet test CodeIndexMcp.slnx -c Release)-parallel none, and under the suite's default parallel-by-collection execution): ratio consistently 0.019-0.027, well under the new 1.0 ceiling