Skip to content

Fix: forward search_buffer_visited_set_ in MutableVamanaIndex batch search - #384

Open
yuejiaointel wants to merge 1 commit into
intel:mainfrom
yuejiaointel:fix/dynamic-index-visited-set-not-forwarded
Open

Fix: forward search_buffer_visited_set_ in MutableVamanaIndex batch search#384
yuejiaointel wants to merge 1 commit into
intel:mainfrom
yuejiaointel:fix/dynamic-index-visited-set-not-forwarded

Conversation

@yuejiaointel

Copy link
Copy Markdown
Contributor

Bug

MutableVamanaIndex's batch search() overload in include/svs/index/vamana/dynamic_index.h constructed its search buffer with only 2 of MutableBuffer's 3 constructor arguments. This left enable_visited at its default (false) regardless of the caller's actual sp.search_buffer_visited_set_ setting — a dead constructor argument. The static VamanaIndex::search() (include/svs/index/vamana/index.h) already forwards this parameter correctly, so this was a static-vs-dynamic inconsistency: visited-set dedup could never be enabled via the dynamic index's batch search path, even when explicitly configured.

Fix

--- a/include/svs/index/vamana/dynamic_index.h
+++ b/include/svs/index/vamana/dynamic_index.h
@@ -534,8 +534,10 @@ class MutableVamanaIndex {
             threads::StaticPartition{queries.size()},
             [&](const auto is, uint64_t SVS_UNUSED(tid)) {
                 size_t num_neighbors = results.n_neighbors();
-                auto buffer =
-                    search_buffer_type{sp.buffer_config_, distance::comparator(distance_)};
+                auto buffer = search_buffer_type{
+                    sp.buffer_config_,
+                    distance::comparator(distance_),
+                    sp.search_buffer_visited_set_};
 
                 auto prefetch_parameters = GreedySearchPrefetchParameters{
                     sp.prefetch_lookahead_, sp.prefetch_step_};

Correctness verification

For both scenarios below, verified bit-identical top-100 result ids before vs. after the fix: 0 mismatches across 5000 queries in each case, checked via direct label-id diffs before trusting any performance number.

Performance impact (measured on dbpedia-1536-900K, deg32/cw200 Vamana graph vs. HNSW M16/efc200, AWS, matched recall)

  • fp32 (gcc-11/13, 16 threads): two independent measurements — +24.73% QPS and a follow-up clean re-test with zero instrumentation confound, +27.08% QPS. Residual gap to HNSW after the fix is ~7.45% (down from ~30-40% before the fix).
  • fp16 (gcc-13, fp16-stored): fix alone gives +19.6% to +21.7% QPS, confirming the bug/fix is dtype-independent. Before the fix, SVS fp16 was roughly at HNSW parity (+0.75% to +2.87%); after the fix, SVS pulls ahead of HNSW by +21.0% to +24.6% at matched recall.

Tests

  • Targeted: ./tests/tests "[dynamic_index],[dynamic_vamana],[graph_index]" — 80797 assertions, 6 test cases, all passing.
  • Broader run — 113126 assertions, 8 test cases, all passing.

…earch

The batch search() overload in MutableVamanaIndex constructed its
search buffer with only 2 of MutableBuffer's 3 constructor arguments,
so enable_visited silently defaulted to false regardless of the
user-configured sp.search_buffer_visited_set_. Visited-set dedup could
never be turned on via this path. The static VamanaIndex::search()
already forwards this parameter correctly (index.h); this makes the
dynamic index's batch search() match that pattern.
@rfsaliev

rfsaliev commented Sep 9, 2026

Copy link
Copy Markdown
Member

This PR duplicates changes made in #364.
There is also the fix which avoids reallocations in case if search config's window_size/buffer_capacity is less than K

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants