Repository navigation
Reduce code duplication between concurrent and non-concurrent indexes - #439
razdoburdin wants to merge 21 commits into
Conversation
…b.com/intel/ScalableVectorSearch into dev/razdoburdin/concurent_unification
ethanglaser
left a comment
ethanglaser
left a comment
There was a problem hiding this comment.
Looking pretty good, a few comments. Ran CI from private repo and no issues.
There was a problem hiding this comment.
can keep this in the first round of cleanup to avoid needed changes in VecSim, but looks like this file could easily be removed entirely and defer to svs::spinlock in VecSim
| #include "svs/index/vamana/multi.h" | ||
|
|
||
| #include <algorithm> | ||
| #include <atomic> |
There was a problem hiding this comment.
Seems like the multi.h are largely duplicated - this is okay for first go around but is there potential to continue to reduce duplication?
There was a problem hiding this comment.
For instance, it looks like MultiVamanaStateLoader is identical - this could be removed now
| ~SpinLock() = default; | ||
| }; | ||
| /// @brief Compatibility alias for svs::SpinLock. | ||
| using SpinLock = svs::SpinLock; |
There was a problem hiding this comment.
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?
| // multiple threads. | ||
| Vector<bool> needs_update_; | ||
| }; | ||
| using vamana::BulkUpdate; |
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:
concurrent/spinlock.h. Theconcurrent::SpinLockis now an alias tolib::SpinLock(VecSim usesconcurrent::SpinLock, so we should keep it). No changes in non-concurrent code.concurrent/translation.h. The file is deleted. The consumers of theconcurrent::IDTranslatornow usesvs::IDTranslator. 3 methods were copy-pasted fromconcurrent::IDTranslatortosvs::IDTranslator.concurrent/iterator.h. Thevamana::concurrent::BatchIteratorclass is kept. The common logic betweenvamana::concurrent::BatchIteratorandvamana::BatchIteratorwas moved tovamana::detail::BatchIteratorBase, whilch is now CRTP base for bothvamana::concurrent::BatchIteratorandvamana::BatchIterator.concurrent/consolidate.h. Theconcurrent::BulkUpdateclass is removed,vamana::BulkUpdateis now used instead of it.conurrent::GraphConsolidator::apply_updatemethod is removed as unused.concurrent/dynamic_search_buffer.h. Theconurrent::MutableBufferinherits now fromvamana::MutableBuffer. Concurrent version contain a bug fix to backport in the non-concurrent index. After the backporting,conurrent::MutableBufferwill be fully replaced byvamana::MutableBuffer.concurrent/graph_concepts.h. Thesvs::graph::MemoryGraphconcept has now an additional template parameter:AddEdgeReturn. For concurrent graph it is used withAddEdgeReturn=concurrent::graphs::AddEdgeResult, for non-concurrent withAddEdgeReturn=size_t.ImmutableMemoryGraphandgraphs_equalare removed. There consumers uses the analogssvs::graph::ImmutableMemoryGraphandsvs::graph::graphs_equal.concurrent/multi.h. Common helpers for assemble from disk and stream are added.concurrent/blocked_data.h.SimpleDataforstd::vectorandlib::SegmentedVectoris unified.concurrent/graph.h. Reuse the same saving/loading logic.concurrent/vamana_build.h. Reusevamana::OptionalTrackerin concurrent code.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.