Skip to content

[SVS][PoC] 2-steps vectors update in SVSTiered#988

Open
rfsaliev wants to merge 4 commits into
mainfrom
rfsaliev/two-step-add-points
Open

[SVS][PoC] 2-steps vectors update in SVSTiered#988
rfsaliev wants to merge 4 commits into
mainfrom
rfsaliev/two-step-add-points

Conversation

@rfsaliev

@rfsaliev rfsaliev commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Proof-of-concept for 2-steps SVS index update

This PR is the PoC to demonstrate potential Tiered SVS index update performance improvements by using 2-steps index update capability implemented in intel/ScalableVectorSearch#348.

Which issues this PR fixes

  1. #...
  2. MOD...

Main objects this PR modified

  1. ...
  2. ...

Mark if applicable

  • This PR introduces API changes
  • This PR introduces serialization changes

Note

Medium Risk
Changes tiered update locking and SVS search/add integration against new prebuilt libraries; incorrect two-phase ordering or ID translation could affect correctness under concurrent queries.

Overview
PoC for shorter exclusive locks during tiered SVS flushes by splitting backend inserts into compute-then-commit using SVS add_points_compute_changes / add_points_commit, wired through new SVSIndexBase::makeAddVectorsChanges and applyAddVectorsChanges.

TieredSVSIndex::updateSVSIndex now runs vector preprocessing and change computation while holding only a shared mainIndexGuard, then upgrades to an exclusive lock solely for applyAddVectorsChanges instead of calling addVectors under the writer lock for the whole operation.

For non-multi SVS indices, single-query TopK avoids the batch QueryResult search API and uses MutableVamanaIndex scratchspace search with translate_internal_id on neighbors.

Build: cmake/svs.cmake points prebuilt SVS downloads at nightly pr330 tarballs, collapses Clang libstdc++ selection to GCC 11+, drops the glibc 2.26 GCC binary path, and raises the minimum glibc for GCC prebuilts to 2.28.

Benchmarks: adds BM_TopKSearchDuringUpdate to stress parallel TopK queries during a background tiered update.

Reviewed by Cursor Bugbot for commit c81cfa8. Bugbot is set up for automated code reviews on this repo. Configure here.

@rfsaliev rfsaliev requested a review from alonre24 July 8, 2026 12:36
@rfsaliev rfsaliev marked this pull request as ready for review July 8, 2026 12:37
svs_index->setParallelism(std::min(availableThreads, labels_to_move.size()));
svs_index->addVectors(vectors_to_move.data(), labels_to_move.data(),
labels_to_move.size());
svs_index->applyAddVectorsChanges(std::move(changes));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend deletes under shared lock

High Severity

updateSVSIndex calls makeAddVectorsChanges while holding only a shared mainIndexGuard lock, but that path still runs deleteVectorsImpl (via impl_->delete_entries) before the exclusive lock and applyAddVectorsChanges. Concurrent backend searches also use shared locks, so they can observe deleted labels and a mutating graph before new points are committed.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit ba22610. Configure here.

@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.80328% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.05%. Comparing base (45daaf3) to head (c81cfa8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/VecSim/algorithms/svs/svs.h 91.22% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
- Coverage   97.12%   97.05%   -0.07%     
==========================================
  Files         141      141              
  Lines        8164     8225      +61     
==========================================
+ Hits         7929     7983      +54     
- Misses        235      242       +7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c81cfa8. Configure here.

if constexpr (!isMulti) {
// SVS index does not support overriding vectors with the same label
// so we have to delete them first if needed
deleted_num = deleteVectorsImpl(labels, n);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Index deletes under shared lock

High Severity

makeAddVectorsChanges calls deleteVectorsImpl, which mutates the SVS backend via delete_entries and markIndexUpdate. Tiered updateSVSIndex invokes this while holding only a shared mainIndexGuard, so concurrent topKQuery paths can read the same impl_ and race with those writes.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c81cfa8. Configure here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant