perf(simq): batch RangeSearch exact reranking - #2692
Conversation
|
/label status/waiting-for-review |
Merge Protections🟢 All 2 merge protections satisfied — ready to merge. Show 2 satisfied protections🟢 Require kind label
🟢 Require version label
|
LHT129
left a comment
There was a problem hiding this comment.
Overall this is a well-executed performance optimization. The change from per-candidate to batched exact reranking in both KnnSearch and RangeSearch is clean and correct.
What I reviewed:
src/algorithm/simq/simq.cpp: BatchedMultiVectorDataCell::Queryin bothKnnSearchandRangeSearch, newrerank_batch_countstatistictests/test_simq.cpp: New test sections for filtering equivalence, all-filtered candidates, and empty radius batch rerankdocs/docs/en/src/indexes/simq.mdanddocs/docs/zh/src/indexes/simq.md: Documentation for the new statistic
Correctness: The batched approach preserves the same filtering, radius, sorting, limited_size, and distance-comparison accounting semantics. The dist_cmp atomic update via fetch_add is correct for the batched path.
Test coverage: The new tests cover the key edge cases — filtering equivalence with KnnSearch, all-filtered candidates producing zero rerank_batch_count, and empty-radius still issuing one batch. The existing require_simq_search_stats helper was updated to assert rerank_batch_count == 1.
Performance: The benchmark data in the PR description shows 4.8x-10.7x latency improvement, which is consistent with reducing N individual io_submit calls to a single batched MultiRead.
One minor note was left as an inline comment about reserve() on in_range.
|
/kind improvement |
Signed-off-by: charlesxu91 <charlesxu.mi@gmail.com> Assisted-by: Codex:gpt-5.5
d510648 to
7b336d3
Compare
LHT129
left a comment
There was a problem hiding this comment.
Re-reviewed the latest commit (7b336d3). The previous inline note about in_range.reserve() has been addressed. No additional findings — the batched reranking implementation in both KnnSearch and RangeSearch is correct, the new rerank_batch_count statistic is properly wired through, and the test coverage for filtering, all-filtered candidates, and empty-radius edge cases is thorough. LGTM.
LHT129
left a comment
There was a problem hiding this comment.
[note] In RangeSearch, the static_cast<uint32_t>(batch_ids.size()) used in stats.dist_cmp.fetch_add() could theoretically truncate the count if batch_ids.size() exceeds UINT32_MAX. While this is not a practical concern given rerank_k limits, using static_cast<uint64_t> (matching the underlying type of dist_cmp if it is std::atomic<uint64_t>) would be more future-proof and consistent with the uint64_t loop counter on the same code path.
Current (src/algorithm/simq/simq.cpp line ~920):
stats.dist_cmp.fetch_add(static_cast<uint32_t>(batch_ids.size()),
std::memory_order_relaxed);Suggestion:
stats.dist_cmp.fetch_add(static_cast<uint64_t>(batch_ids.size()),
std::memory_order_relaxed);The same pattern also appears in KnnSearch at approximately line 800.
LHT129
left a comment
There was a problem hiding this comment.
[note] Regarding the earlier suggestion to change static_cast<uint32_t> to static_cast<uint64_t> in stats.dist_cmp.fetch_add() (both in KnnSearch ~line 800 and RangeSearch ~line 920): the underlying type of dist_cmp is std::atomic<uint32_t> (defined in src/query_context.h). The fetch_add parameter type should match the atomic's value type, so static_cast<uint32_t> is correct here. Using uint64_t would cause an implicit narrowing conversion. No change needed.
Otherwise, this PR is well-executed: the batched reranking logic is correct in both KnnSearch and RangeSearch, the new rerank_batch_count statistic is properly wired through, and the test coverage for filtering, all-filtered candidates, and empty-radius edge cases is thorough. The in_range.reserve() fix from the earlier review round is already applied. LGTM.
LHT129
left a comment
There was a problem hiding this comment.
Thanks for this PR. The change is clean, well-scoped, and the performance improvement is substantial (3-10x).
What I reviewed:
src/algorithm/simq/simq.cpp: The batchedQuerycall inRangeSearchis correctly implemented, mirroring the existing pattern inKnnSearch. Thein_range.reserve()fix from the earlier review has been applied. Statistics plumbing forsimq_rerank_batch_countis consistent across both search paths and the dump helper.tests/test_simq.cpp: Test coverage is thorough — exact equivalence with filtered KNN,limited_sizetruncation, all-filtered candidates (verifying zero IO), and empty-radius edge case. TheEvenLabelFilterandRejectAllFilterhelpers are minimal and well-placed.docs/docs/{en,zh}/src/indexes/simq.md: Documentation additions are accurate and match the implementation.
No blocking issues found. The batched rerank approach is a straightforward and effective optimization.
LHT129
left a comment
There was a problem hiding this comment.
I reviewed this PR and the changes look solid. The batch reranking approach in RangeSearch is consistent with the existing pattern in KnnSearch and the performance gains are well-documented.
What was checked:
- Correctness: The batched
Querycall replaces per-candidate calls while preserving radius filtering, result ordering,limited_size, filter behavior, and distance-comparison accounting. Thererank_batch_countstatistic is correctly set to 0 when no candidates survive filtering and 1 otherwise. - Memory:
batch_idsandbatch_distsare request-local.in_range.reserve(batch_ids.size())is correctly placed inside the!batch_ids.empty()guard (addressing the prior review comment). - Concurrency:
stats.dist_cmp.fetch_addwithmemory_order_relaxedis appropriate for statistics counters. No shared mutable state was added. - Test coverage: The new test sections cover filtering + limited_size equivalence with KnnSearch, all-filtered candidates (rerank_batch_count == 0, dist_cmp == 0), and empty radius (still batches, produces zero results). The
EvenLabelFilterandRejectAllFilterhelpers are minimal and correct. - Documentation: Both English and Chinese SIMQ docs are updated with the new statistic key.
- Style: Follows VSAG conventions (4-space indent,
uint64_t,vsagnamespace,.cppsuffix).
No blocking issues found.
LHT129
left a comment
There was a problem hiding this comment.
Code review summary:
This PR batches the SIMQ RangeSearch exact-rerank path (previously one MultiVectorDataCell::Query per candidate) into a single batched call, mirroring the pattern already used in KnnSearch. The change is clean and well-tested.
What was checked:
- Correctness: The batched
Querycall, radius filtering, result ordering,limited_sizetruncation, and filter integration are all correctly preserved. Thererank_batch_countstat (0 or 1) is computed correctly for both empty and non-empty batch cases. - Consistency: The batched pattern in
RangeSearchmatches the existingKnnSearchimplementation exactly — samebatch_idscollection, sameQuerycall signature, samefetch_addondist_cmp. - Statistics: All call sites of
dump_simq_statisticsare updated with the newrerank_batch_countparameter (4 call sites acrossKnnSearchandRangeSearch). - Tests: Comprehensive regression coverage including exact equivalence with
KnnSearch, filtering behavior, all-filtered candidates (verifying zero IO), empty radius, andlimited_size. Therequire_simq_search_statshelper was updated to assertrerank_batch_count == 1. - Documentation: Both English and Chinese SIMQ docs updated with the new stat key.
- Performance: The PR description includes detailed before/after benchmarks showing 3-10x latency improvements.
No blocking issues found. The previously noted in_range.reserve() suggestion has been addressed in the current diff.
LHT129
left a comment
There was a problem hiding this comment.
I have reviewed this PR thoroughly. The changes are clean and well-structured.
Summary of changes:
RangeSearchnow collects all filter-passing candidates and performs one batchedMultiVectorDataCell::Querycall instead of per-candidate calls, matching the pattern already used inKnnSearch.- Added
simq_rerank_batch_countsearch statistic (0 when no batch needed, 1 otherwise). - Added comprehensive regression tests covering exact result equivalence with filtering,
limited_size, all-filtered candidates, and empty radius. - Updated English and Chinese SIMQ documentation with the new statistic.
Review findings:
- No correctness issues found. The batched
Querycall uses the correct API signature fromFlattenInterface. - The
stats.dist_cmpatomic increment pattern is consistent betweenKnnSearchandRangeSearch. - The
in_range.reserve(batch_ids.size())optimization (from a prior review comment) is correctly applied. - All call sites of
dump_simq_statisticsare updated with the new parameter. - Test coverage is thorough, covering the key edge cases (all-filtered, empty radius, filtering equivalence).
- The performance data in the PR description shows significant improvements (4.8x-10.7x p50 latency reduction).
No blocking issues or suggestions to raise. The PR is ready for merge from a code review perspective.
LHT129
left a comment
There was a problem hiding this comment.
I completed a thorough review of this PR. Here is my assessment:
Summary: This PR batches the exact-rerank MultiVectorDataCell::Query calls in both KnnSearch and RangeSearch, replacing per-candidate calls with a single batched call. The performance gains are substantial (4.8x-10.67x latency improvement). The change is well-scoped, correctly preserves all existing behavior (filtering, radius, limited_size, sorting, distance accounting), and includes comprehensive regression tests.
What I checked:
- Correctness: The batched
Querycall signature and usage match the existing single-candidate pattern.stats.dist_cmpis correctly updated withfetch_addfor the batch size. Filtering logic is unchanged — candidates are collected intobatch_idsonly after passing the filter check. - Edge cases: Empty
batch_ids(all candidates filtered) is handled correctly —rerank_batch_countis 0 and theQuerycall is skipped entirely. Thein_range.reserve(batch_ids.size())fix from the earlier review comment is present. - Test coverage: Three new test sections cover filtered results matching thresholded KNN, all-filtered candidates (verifying zero IO), and empty-radius reranking. The existing
require_simq_search_statshelper correctly assertsrerank_batch_count == 1. - Documentation: Both English and Chinese docs are updated with the new statistic key.
- Performance: The benchmark data in the PR description is thorough and shows consistent gains across different
rerank_kvalues.
No blocking issues found. The one prior review comment about in_range.reserve() has been addressed. The code is clean, well-tested, and follows VSAG conventions.
LHT129
left a comment
There was a problem hiding this comment.
Reviewed the batched RangeSearch exact-rerank change. The implementation correctly mirrors the existing batched approach in KnnSearch:
- Correctness: Filtering, radius check,
limited_sizetruncation, sorting, and distance-comparison accounting are all preserved. Thein_range.reserve()was added per prior review feedback. - Test coverage: Comprehensive — exact equivalence with thresholded KNN, filtering,
limited_size, all-filtered candidates (no IO), and empty radius edge case. All stats invariants are verified. - Statistics:
simq_rerank_batch_countis correctly emitted as 0 or 1, and documented in both English and Chinese docs. - Performance: The batched
MultiVectorDataCell::Querycall enablesMultiReadin the underlying data cell, which aligns with the measured ~5-10x latency improvement.
No blocking issues found. The change is consistent with the existing codebase style and the prior KnnSearch batching pattern.
LHT129
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR batches the SIMQ RangeSearch exact-rerank step into a single MultiVectorDataCell::Query call, delivering substantial latency and throughput improvements (up to 10.67x p50, 3.67x QPS in benchmarks).
What was reviewed
src/algorithm/simq/simq.cpp— core batching logic in bothKnnSearchandRangeSearchtests/test_simq.cpp— new regression testsdocs/docs/en/src/indexes/simq.mdanddocs/docs/zh/src/indexes/simq.md— documentation
Findings
Correctness: The batching transformation is sound. The filter-then-batch pattern in RangeSearch correctly mirrors the existing KnnSearch pattern. The dist_cmp counter is updated atomically with fetch_add, consistent with KnnSearch. All dump_simq_statistics call sites are correctly updated for the new parameter.
Edge cases covered by tests:
- Exact result equivalence with filtering and
limited_size - All-filtered candidates (batch count = 0, no IO)
- Empty radius (batch count = 1, zero results)
- Statistics assertions in the existing
require_simq_search_statshelper
Performance: The in_range.reserve(batch_ids.size()) call (added per prior review feedback) avoids reallocations. No unnecessary copies or allocations in the hot path.
Style: Follows VSAG conventions — 4-space indent, uint64_t usage, vsag namespace, .cpp suffix.
No blocking issues found. The change is low-risk, well-tested, and the performance data is compelling.
LHT129
left a comment
There was a problem hiding this comment.
Thanks for this PR. The batched exact-rerank approach for RangeSearch is a clean improvement that aligns it with the existing KnnSearch pattern.
Summary of changes:
RangeSearchnow collects all filter-surviving candidates and issues a single batchedMultiVectorDataCell::Querycall instead of one per candidate.- New
simq_rerank_batch_countstatistic (0 when no batch is needed, otherwise 1). - Three new test cases covering filtering +
limited_size, all-filtered candidates, and empty radius. - Documentation updated in both English and Chinese.
Review notes:
- The
in_range.reserve(batch_ids.size())suggestion from LHT129 has been addressed. - The
static_cast<InnerIdType>(batch_ids.size())narrowing note from LHT129 is a minor portability concern — not a practical issue given realistic batch sizes. - Test coverage is thorough: exact equivalence with
KnnSearch, filtering,limited_size, all-filtered edge case, and empty radius. - The performance data in the PR description is compelling (up to 10.67x p50 improvement).
No blocking issues found. The change is consistent with the existing KnnSearch batching pattern and the code is clean.
LHT129
left a comment
There was a problem hiding this comment.
I have completed a thorough review of this PR. The changes are well-structured and the code quality is high.
Summary of changes:
simq.cpp: BatchedRangeSearchexact reranking viaMultiVectorDataCell::Queryinstead of per-candidate calls. The same pattern is already used inKnnSearchand this PR extends it toRangeSearch.test_simq.cpp: Comprehensive regression tests covering exact result equivalence with filtering,limited_size, all-filtered candidates (batch count = 0), and empty radius (batch count = 1).- Documentation: New
simq_rerank_batch_countstatistic documented in both English and Chinese docs.
Review findings:
- No correctness issues found. The batched approach preserves radius filtering, result ordering,
limited_size, filter behavior, and distance-comparison accounting. - The
in_range.reserve(batch_ids.size())fix (from the earlier review comment) is correctly applied. - Test coverage is thorough, covering normal paths, filtering edge cases, and the all-filtered / empty-radius boundary conditions.
- The performance data in the PR description shows significant improvements (4.8x-10.7x p50 latency reduction), which aligns with the architectural change.
The existing inline comment from @LHT129 about static_cast<InnerIdType>(batch_ids.size()) narrowing is a valid portability note but not blocking given practical batch sizes.
No additional issues to raise. LGTM.
LHT129
left a comment
There was a problem hiding this comment.
Reviewed the latest diff (commit 7b336d3). This PR is well-structured and the changes are correct:
What was reviewed:
src/algorithm/simq/simq.cpp: RangeSearch batching logic mirrors the existing KnnSearch pattern. Thein_range.reserve()call is now present (addressed from prior review). Thedist_cmpaccounting viafetch_addcorrectly reflects all computed distances. Early-return paths correctly pass the newrerank_batch_countparameter.tests/test_simq.cpp: Four new test sections provide good coverage — equivalence with thresholded KNN under filtering,limited_sizetruncation, all-filtered candidates (batch_count=0, no IO), and empty radius (batch_count=1, zero results). TheEvenLabelFilterandRejectAllFilterhelpers are clean and minimal.docs/docs/{en,zh}/src/indexes/simq.md: New statistic documented in both languages.
No new issues found. The two points raised in prior review comments (reserve and narrowing cast) have been addressed or noted. The performance data in the PR description shows 3-10x improvement, which is consistent with the reduction from N individual Query calls to 1 batched call.
LHT129
left a comment
There was a problem hiding this comment.
Reviewed the batched RangeSearch exact reranking change. The implementation is clean and well-tested.
Summary of findings:
-
The core change — collecting filtered candidates into
batch_idsand issuing a single batchedMultiVectorDataCell::Querycall — is correct and preserves all existing behavior (radius filtering, result ordering,limited_size, filter semantics, distance-comparison accounting). -
The
in_range.reserve(batch_ids.size())call was added in response to a prior review comment, which is good — it avoids reallocations during theemplace_backloop. -
The
static_cast<InnerIdType>(batch_ids.size())narrowing (line 917 inRangeSearch, also present inKnnSearchat line 797) has already been noted in a prior review comment. This is a pre-existing pattern in the codebase and is practically safe given realistic batch sizes. -
The new test coverage is thorough: exact result equivalence with filtered KnnSearch,
limited_sizetruncation, all-filtered candidates (verifying zero IO), and empty-radius behavior. TheEvenLabelFilterandRejectAllFiltertest helpers are minimal and focused. -
The documentation updates in both
enandzhSIMQ docs correctly describe the newsimq_rerank_batch_countstatistic. -
The performance data in the PR description shows significant improvements (3-10x latency reduction), which aligns with the expected benefit of batching IO calls.
No blocking issues found. The PR is ready for merge from a code review perspective.
LHT129
left a comment
There was a problem hiding this comment.
I have completed a thorough review of this PR. The changes are well-structured and correct:
Summary of changes:
- Batched
RangeSearchexact reranking viaMultiVectorDataCell::Query(matching the existingKnnSearchpattern) - Added
simq_rerank_batch_countstatistic - Comprehensive test coverage for filtering,
limited_size, all-filtered candidates, and empty radius - Documentation updated in both English and Chinese
Review findings:
- The batching logic is correct: candidate IDs are collected after filtering, then submitted in a single
Querycall - Radius filtering is properly preserved in the post-Query loop
dist_cmpis correctly updated withfetch_addfor the batch sizererank_batch_countcorrectly reports 0 when all candidates are filtered and 1 otherwise- The two issues previously raised by @LHT129 (
in_range.reserve()andstatic_castnarrowing) have been addressed - No new issues found
The performance improvements (4.8x-10.67x latency reduction) are consistent with the reduction in io_submit calls. LGTM.
LHT129
left a comment
There was a problem hiding this comment.
I have completed a thorough review of this PR. Here is my assessment:
Summary: This PR cleanly refactors SIMQ RangeSearch (and KnnSearch) to batch exact-rerank calls into a single MultiVectorDataCell::Query invocation, yielding significant performance gains (up to 10.67x p50 improvement at rerank_k=1000). The changes are minimal, well-scoped, and preserve all existing behavior (result ordering, radius filtering, limited_size, filter semantics, distance-comparison accounting).
What was reviewed:
src/algorithm/simq/simq.cpp: The batching logic in bothKnnSearchandRangeSearchis consistent and correct. Filter-then-batch-then-query is the right order. Thererank_batch_countstatistic is properly computed (0 when all candidates are filtered, 1 otherwise).tests/test_simq.cpp: New test sections cover exact result equivalence with filtering,limited_sizeinteraction, all-filtered candidates (verifying zero IO), and empty radius (verifying batch still executes). TheEvenLabelFilterandRejectAllFilterhelpers are clean. Existing tests pass with the newrerank_batch_countassertion.docs/docs/{en,zh}/src/indexes/simq.md: Documentation updates for the new statistic are accurate in both languages.
Findings: No blocking issues, no logic errors, no memory safety concerns. The code follows VSAG conventions (4-space indent, uint64_t, vsag namespace, .cpp suffix). The pre-existing std::move on return in RangeSearch (line 956) and the a.first < b.first sort lambda (lines 937-939) are unchanged by this PR and are outside its scope.
Verdict: This is a well-executed performance improvement with solid test coverage. No changes requested.
LHT129
left a comment
There was a problem hiding this comment.
Reviewed the batch RangeSearch exact reranking changes. The refactor correctly consolidates per-candidate MultiVectorDataCell::Query calls into a single batched call, matching the pattern already used in KnnSearch. The logic is sound, the performance gains are substantial (4.8x–10.67x p50), and the new test coverage (filtering equivalence, all-filtered avoidance, empty radius) covers the key edge cases. The in_range.reserve() fix from the earlier review round is in place. No blocking issues found.
LHT129
left a comment
There was a problem hiding this comment.
Reviewed the batch RangeSearch exact reranking changes. The implementation is clean and correct:
RangeSearchnow collects all filter-passing candidates intobatch_idsand issues a single batchedMultiVectorDataCell::Querycall, matching the pattern already used inKnnSearch.in_range.reserve(batch_ids.size())is properly called before the emplace_back loop (addressed from prior review).stats.dist_cmpis updated withfetch_addusingmemory_order_relaxed, consistent with theKnnSearchpath.- The
rerank_batch_countstatistic is correctly computed as 0 when no candidates survive filtering and 1 otherwise. - Edge cases are handled: empty index, all-filtered candidates (no rerank IO), empty radius (still reranks),
limited_sizetruncation. - Tests cover exact result equivalence with filtered KNN, all-filtered candidate avoidance, and empty-radius reranking.
- Documentation is updated in both English and Chinese.
No new issues found. LGTM.
LHT129
left a comment
There was a problem hiding this comment.
Review Summary
This PR batches the SIMQ RangeSearch exact reranking by collecting all filter-surviving candidates and issuing a single MultiVectorDataCell::Query call instead of one per candidate. The same batching pattern is also applied to KnnSearch for consistency.
Correctness: The logic is sound. The two-pass approach (collect valid IDs → batched Query → radius filter) preserves the original semantics. The early-return paths correctly pass the new rerank_batch_count parameter. The dist_cmp accounting is correctly updated to reflect the batch size.
Performance: The measured improvements (4.8x–10.7x latency reduction) are consistent with the expected reduction in io_submit calls. The batch approach is the right optimization for this hot path.
Testing: Comprehensive — covers exact result equivalence against KnnSearch with filtering, limited_size truncation, all-filtered candidates (zero batch), and empty radius (one batch with zero results). The new simq_rerank_batch_count statistic is validated in all test paths.
Documentation: Both English and Chinese docs are updated with the new statistic.
No blocking issues found. The code is clean, well-tested, and the performance data is thorough.
Change Type
Linked Issue
Not required for
kind/improvement.What Changed
RangeSearchcandidates that survive filtering and exact-rerank them with one batchedMultiVectorDataCell::Querycall instead of one call per candidate.limited_size, filter behavior, and distance-comparison accounting.simq_rerank_batch_countsearch statistics (0when no batch is needed, otherwise1).limited_size, all-filtered candidates, and an empty result radius.Test Evidence
make fmtmake lintmake testmake cov, run tests, and collect coverageTest details:
The exact four committed file hashes match the tree used for the tests above. An independent code-review pass reported no findings.
Performance
Environment: AMD Ryzen 7 9700X, Ubuntu 24.04.4, 1,000 documents x 8 token vectors, 100 queries x 4 token vectors, 128 dimensions, inner product,
coarse_k=10, five warmups. Results are medians of three independent before and after processes.rerank_krerank_k=1000syscall sample reducedio_submitcalls from 5,900 to 60 (98.98%) with the same result checksum.nice/ionice; no resident service was stopped and caches were not dropped. Whole-machine average idle during latency monitoring was 98.13%.TARGET=ZENbecause bundled OpenBLAS 0.3.24 selected an incompleteAMD_UNKNOWNconfiguration. This target-only build adjustment is not part of the diff.Compatibility Impact
Performance and Concurrency Impact
Documentation Impact
README.mdDEVELOPMENT.mdCONTRIBUTING.mddocs/docs/{en,zh}/src/indexes/simq.mdRisk and Rollback
Checklist
kind/improvement.