Skip to content

Reduce code duplication between concurrent and non-concurrent indexes - #439

Open
razdoburdin wants to merge 24 commits into
mainfrom
dev/razdoburdin/concurent_unification
Open

razdoburdin wants to merge 24 commits into
mainfrom
dev/razdoburdin/concurent_unification

Conversation

@razdoburdin

@razdoburdin razdoburdin commented Oct 20, 2026 •

Copy link
Copy Markdown
Member

Follow-up the #369

This PR unify the code between concurrent and non-concurrent implementation of MutableVamana. The goal is to reuse code without any behavioral changes.

Changes list:

  1. concurrent/spinlock.h. The concurrent::SpinLock is now an alias to lib::SpinLock (VecSim uses concurrent::SpinLock, so we should keep it). No changes in non-concurrent code.
  2. concurrent/translation.h. The file is deleted. The consumers of the concurrent::IDTranslator now use svs::IDTranslator. 3 methods were copy-pasted from concurrent::IDTranslator to svs::IDTranslator.
  3. concurrent/iterator.h. The vamana::concurrent::BatchIterator class is kept. The common logic between vamana::concurrent::BatchIterator and vamana::BatchIterator was moved to vamana::detail::BatchIteratorBase, whilch is now CRTP base for both vamana::concurrent::BatchIterator and vamana::BatchIterator.
  4. concurrent/consolidate.h. The concurrent::BulkUpdate class is removed, vamana::BulkUpdate is now used instead of it. conurrent::GraphConsolidator::apply_update method is removed as unused.
  5. concurrent/dynamic_search_buffer.h. The conurrent::MutableBuffer inherits now from vamana::MutableBuffer. Concurrent version contain a bug fix to backport in the non-concurrent index. After the backporting, conurrent::MutableBuffer will be fully replaced by vamana::MutableBuffer.
  6. concurrent/graph_concepts.h. The svs::graph::MemoryGraph concept has now an additional template parameter: AddEdgeReturn. For concurrent graph it is used with AddEdgeReturn = concurrent::graphs::AddEdgeResult, for non-concurrent with AddEdgeReturn = size_t. ImmutableMemoryGraph and graphs_equal are removed. There consumers uses the analogs svs::graph::ImmutableMemoryGraph and svs::graph::graphs_equal.
  7. concurrent/multi.h. Common helpers for assemble from disk and stream are added.
  8. concurrent/blocked_data.h. SimpleData for std::vector and lib::SegmentedVector is unified.
  9. concurrent/graph.h. Reuse the same saving/loading logic.
  10. concurrent/vamana_build.h. Reuse vamana::OptionalTracker in concurrent code.
  11. concurrent/dynamic_index.h. Reuse the same saving/loading and calibration logic.

Next iteration of unification will be done, after backporting of the minor fixes from concurrent index.

@razdoburdin
razdoburdin marked this pull request as draft October 20, 2026 13:15
@razdoburdin
razdoburdin marked this pull request as ready for review October 26, 2026 14:16
@razdoburdin
razdoburdin removed the request for review from yuejiaointel October 26, 2026 14:16
@razdoburdin
razdoburdin enabled auto-merge (squash) October 27, 2026 08:24

@ethanglaser ethanglaser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking pretty good, a few comments. Ran CI from private repo and no issues.

Comment thread include/svs/concurrent/spinlock.h
Comment thread include/svs/concurrent/multi.h
~SpinLock() = default;
};
/// @brief Compatibility alias for svs::SpinLock.
using SpinLock = svs::SpinLock;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The concurrent README states concurrent spinlock adds move/copy functionality which would be lost with this (https://github.com/intel/ScalableVectorSearch/blob/main/include/svs/concurrent/README.md?plain=1#L127) - is it worth trying to preserve or should the README be updated?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Readme should be updated. I think all the refactoring stages should be done first.

Comment thread include/svs/concurrent/consolidate.h Outdated

This branch has not been deployed

No deployments
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