fix: forward + default-enable search_buffer_visited_set_ in concurrent MutableVamanaIndex - #390
Open
yuejiaointel wants to merge 2 commits into
Conversation
…dex batch search The batch search() in svs::index::vamana::concurrent::MutableVamanaIndex constructed search_buffer_type with only 2 args (buffer_config_, comparator), dropping the 3rd constructor arg (enable_visited). As a result sp.search_buffer_visited_set_ never took effect on this index's batch search path, identical to a bug already fixed upstream in the non-concurrent MutableVamanaIndex::search (intel/ScalableVectorSearch commit 8052bd9, PR intel#364, merged 2026-09-11). This file (include/svs/concurrent/dynamic_index.h) is a near-verbatim copy carried into a new namespace by PR intel#369, branched 2026-08-20 -- before 8052bd9 landed -- so it did not inherit the fix. The single-query search path (scratchspace()) in this same file already forwards the flag correctly; only the batch path was missing it. Verified: razdoburdin/ScalableVectorSearch's seqlock branch (the source PR intel#369 ports from) also lacks 8052bd9 and has the identical unfixed 2-arg construction in its own dynamic_index.h, confirming this bug was never independently fixed upstream and a rebase would not resolve it (the fix in main touches a different file than this copy). Verification done: tests/svs/concurrent/dynamic_index.cpp and dynamic_index_2.cpp (the tests covering this class) both compile cleanly with g++-11 after this change. Full test-suite link/run was not performed (out of scope for this pass).
Mirrors the already-open (unmerged) intel/ScalableVectorSearch PR intel#389 (feature/visited-set-default-on) against main, same evidence: with the forwarding bug fixed (previous commit here, and upstream commit 8052bd9 / PR intel#364 for the non-concurrent index), a 48-config regression campaign (2 datasets x 2 modes x 3 metrics x 4 variants, k=100, recall 0.90-0.95) showed a consistent 1.2x-1.4x QPS gain with zero recall cost and zero regressions. Flipping the default means callers -- including this PR's new concurrent::MutableVamanaIndex -- get the benefit without needing to know the flag exists. Note: svs::index::vamana::concurrent::MutableVamanaIndex does not have its own search-parameters type -- it reuses VamanaSearchParameters from include/svs/index/vamana/search_params.h directly (see svs::index::vamana::concurrent::MutableVamanaIndex::search_parameters_type alias). That file is untouched/unforked by this PR (intel#369), and is byte-identical here to current intel/ScalableVectorSearch main. So this one edit is not concurrent-specific: it flips the default for the existing (non-concurrent) index too, identically to what PR intel#389 already proposes on main. The two changes are currently on unrelated branches/bases; if intel#389 merges to main before this PR does, rebasing this branch onto main will make this commit a no-op (already applied upstream) rather than a conflict, since the resulting text is identical. Verified: tests/svs/concurrent/dynamic_index.cpp, tests/svs/concurrent/dynamic_index_2.cpp, tests/svs/index/vamana/dynamic_index.cpp, and tests/svs/index/vamana/search_parameters.cpp all recompile cleanly with g++-11 after this change (confirmed via fresh object-file timestamps, not stale artifacts). Full test-suite link/run not performed.
|
Tick the box to add this pull request to the merge queue (same as
|
mihaic
approved these changes
Sep 21, 2026
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
2 participants
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.
What
Two changes, both against PR #369's new
svs::index::vamana::concurrent::MutableVamanaIndex:include/svs/concurrent/dynamic_index.h's batch-search path constructedsearch_buffer_typewith only 2 args (buffer_config_, comparator), silently dropping the 3rd arg (enable_visited) — sosearch_parameters.search_buffer_visited_set_never took effect for batch search. Mirrors the already-merged fix for the non-concurrent index (commit 8052bd9, PR [C API] Implement memory estimation functions for index build and search #364).include/svs/index/vamana/search_params.h'ssearch_buffer_visited_set_default changed fromfalsetotrue. This struct (VamanaSearchParameters) is shared/unforked by PR separate seqlock fine grain into separate index #369 (concurrent::MutableVamanaIndex::search_parameters_typeis a direct alias to it), so this one edit affects both the concurrent and non-concurrent index identically.Why
The visited-set feature has been validated in a large regression campaign (2 datasets x 2 modes x 3 metrics x 4 variants = 48 configs, k=100, recall 0.90-0.95): a consistent 1.2x-1.4x QPS gain, zero recall cost, zero regressions. Flipping the default means callers get the benefit without needing to know the flag exists.
Note on duplication with #389
#389("Change search_buffer_visited_set_ default from false to true") already proposes the identical default-flip change againstmain, opened separately today. The diff text here is copied verbatim from #389 for consistency — if #389 merges tomainbefore this PR's base branch does, rebasing this branch will make that commit a no-op (identical resulting text), not a conflict.Verification
tests/svs/concurrent/dynamic_index.cpp,dynamic_index_2.cpp,tests/svs/index/vamana/dynamic_index.cpp, andsearch_parameters.cppall recompile cleanly with g++-11 after both changes (confirmed via fresh object-file timestamps).🤖 Generated with Claude Code