separate seqlock fine grain into separate index - #369
Draft
ethanglaser wants to merge 1 commit into
Draft
Conversation
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
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.
main...razdoburdin:ScalableVectorSearch:seqlock without modifying existing index
What this PR does
Reproduces the functionality of
razdoburdin:ScalableVectorSearch:seqlock— lock-free search concurrent withadd_points,delete_entries, andconsolidate— as a separate index type,svs::index::vamana::concurrent::MutableVamanaIndex, rather than by modifying the existing one.The source branch (referred to below as
seqlock) implements this by editing 22 headers in place; the static and dynamic indexes share most of them. This PR is additive only — ~12.2k insertions, 0 deletions, and one pre-existing file touched (tests/CMakeLists.txt, to register tests). Everything lives insvs::index::vamana::concurrent, a namespace nested inside the one it shadows, so entities this stack does not change resolve to their existing upstream declarations.Design notes, lock discipline, and the divergence list are in
include/svs/concurrent/README.md.How the diff was verified against
seqlockFor every ported file, namespace names, include paths,
concurrent::qualification, comments, and blank lines were normalized away, then diffed against theseqlockversion. What survives is functional difference. Across the 11 ported headers, 4,394 code lines compared, 212 lines of residual — 95% line-for-line identical.Implementation
seqlockseqlocklib/segmented_vector.h(330)lib/concurrency/seqlock.h(141)lib/concurrency/atomic_span.h(93)concurrent/consolidate.h(525)index/vamana/consolidate.h(+239)concurrent/multi.h(967)index/vamana/multi.h(+124)concurrent/prune.h(338)index/vamana/prune.h(+89)concurrent/vamana_build.h(605)index/vamana/vamana_build.h(+32)concurrent/dynamic_search_buffer.h(625)index/vamana/dynamic_search_buffer.h(+8)MutableBufferconcurrent/graph_concepts.h(222)concepts/graph.h(+22)AddEdgeResultand theMemoryGraphconcept are identical)concurrent/translation.h(491)core/translation.h(+77)namespacedeclarationconcurrent/reverse_edges.h(99)core/graph/reverse_edges.h(+99)concurrent/iterator.h(358)index/vamana/iterator.h(+117)unsafe_translate_internal_idunder the caller's lock —std::shared_mutexis not recursive, and the source branch re-acquires it per neighborconcurrent/graph.h(677)core/graph/graph.h(+240)std::atomic_refadjacency-slot accesses route throughrelaxed_load/relaxed_store— same relaxed ops, one gate for the TSan negative control; (b)SegmentedBlockedDatain place of the modifiedBlockedData; (c)rebuild_reverse_edgesrecords unconditionally (fix 1 below)concurrent/greedy_search.h(203)index/vamana/greedy_search.h(+85)using-declarations replace verbatim copies ofNullTracker,GreedySearchPrefetchParameters,EntryPointInitializer,NeighborBuilder,GreedySearchTracker. Redeclaring an unchanged entity inside the nested namespace creates a distinct look-alike type that breaks conversions with upstream (e.g.SearchScratchspace::prefetch_parameters). The SeqLock retry loop itself is identicalconcurrent/dynamic_index.h(2080)index/vamana/dynamic_index.h(+681)foo/unsafe_foo(fix 2); (b)supplement_search_buffer+ asingle_searchsvs_invokeoverride in place of the source branch's edit toindex/vamana/extensions.h; (c) 3 qualification linesconcurrent/blocked_data.h(433)core/data/simple.h(+13)std::vector→lib::SegmentedVectorblock-directory swap the source branch applies toSimpleData<T, Extent, Blocked<Alloc>>, applied instead to a newSegmentedBlocked<Alloc>allocator tag, so no existing dataset type changes. Result is still aSimpleData, so the dataset concepts,extensions,compact_data, and the serializer all apply unchanged. Also adds release/acquire onsize_(fix 3)concurrent/spinlock.h(49)lib/spinlock.h(+7)SegmentedVector<SpinLock>needs them), added by subclassing rather than by editinglib/spinlock.hconcurrent/concurrent.h(54)svs::concurrentnamespace alias. The source branch needs no analogue because it edits upstream in placeconcurrent/README.mdTests and build
seqlockseqlocktests/svs/lib/segmented_vector.cpp(237)tests/svs/concurrent/translation.cpp(288)tests/svs/core/translation.cpp(+40)[concurrent]tags, type aliases,SegmentedBlockedDataforBlockedData, and a local data loader sotests/utils/needs no edittests/svs/concurrent/dynamic_index.cpp(548)tests/svs/index/vamana/dynamic_index.cpp(+108)auto_dynamic_assembleinstead ofsvs::DynamicVamana::assemble— the type-erased orchestrator is not wired to the new index (see Scope below)tests/svs/concurrent/{graph,prune,consolidate,iterator,multi,dynamic_index_2}.cpp(1,894)prune.cppasserts the newreenablesemantics (fix 4);dynamic_index_2.cppadditionally scopes its helper block in an anonymous namespace (ODR: the upstream file declares the sameReportand its stream operator) and scopes its global-logger sink push with aScopedGlobalSinkguard (the upstream version leaks a sink that outlives, by reference, the vector it captures)tests/svs/concurrent/concurrency.cpp(578)tests/CMakeLists.txt(+75)Intentional functional divergences
These are the only places where behaviour differs from
seqlock. All four are defects in the source branch; each is documented at its site and in the README.rebuild_reverse_edgesrecords every edge.seqlockskips edges whose reverse also exists, halving the index on the reasoning thatgather_work_setvisitsout(d) ∪ R(d). That invariant holds right after a rebuild but is not maintainable: the moment consolidation rewiresdand dropsd -> u,u's in-edge becomes invisible, and a later deletion ofdleavesupointing at a retired slot. It is masked inseqlockbecause every one of its tests pairsconsolidate()withcompact(), which rebuilds from scratch. Repeatedconsolidate()withoutcompact()— the cheap maintenance path — corrupts the graph within two rounds (debug_check_invariants()reportsNode number N has an invalid (Empty) neighbor). Recording unconditionally gives the strictly strongerR(d) ⊇ in(d), which no edge removal can break.translate_external_id,translate_external_id_or,has_id,translate_internal_id, andon_idsread thetsl::robin_maps with no lock inseqlockwhileadd_pointsinserts andconsolidateerases undertranslator_mutex_. The hazard is not a stale read: an insert can rehash and free the bucket array a reader is walking. TSan reports 24 races on the original.size_is published with release/acquire. It is written byresize()while lock-free searches read it viasize(); both are plain accesses inseqlock. TSan reports it on any search overlapping anadd_points.PruneStatetest expectations.seqlockredefinesreenableto promoteCandidaterather thanPrunedback toAvailable, but leaves the pre-existingtests/svs/index/vamana/prune.cppassertingreenable(Pruned) == Available. Here the concurrentPruneStateis a distinct type in a distinct namespace, so the upstream test keeps passing against upstreamreenable, and the ported test asserts the new semantics.Three changes in
seqlockare deliberately not ported, none of which affect index behaviour:NullLockGuardinindex/vamana/index.h(only needed becauseseqlockshares oneBatchIteratorwith the static index; this stack has its own),capacity()inquantization/scalar/scalar.h(memory reporting, unused here), and the Python binding/GIL changes.Testing
ctest -L "concurrent|segmented_vector"debug_check_invariants(), post-mutation recall > 0.85,consolidate()+compact(), re-checkThreadSanitizer
Correctness here rests almost entirely on memory ordering, which an uninstrumented test can only fail to disprove:
concurrent_tsanmust come out clean (it does — 0 warnings).concurrent_tsan_negativeis a negative control, registeredWILL_FAIL TRUE: it definesSVS_CONCURRENT_UNSAFE_PLAIN_GRAPH_ACCESS, degrading the adjacency-slot accessors to plain loads and stores, and reports 54 races, all inrelaxed_load/relaxed_store. A clean positive run only means something if the same run fires once the atomics are removed.Scope
include/svs/concurrent/references the new index: nosvs::DynamicVamanaorchestrator support, no Python bindings, no benchmark integration. Withseqlock, existing callers get concurrency implicitly; here they opt into the new type from C++. This is a direct consequence of not modifying the existing index.seqlockor against the existing dynamic index.std::atomic_refis absent from libc++ before LLVM 19 (seqlockhas the same exposure), and every concurrency claim so far is validated on x86's strong memory model.concurrency.cpp;MultiMutableVamanaIndexis ported but has no concurrent test. Dataset coverage isfloat32SegmentedBlockedDataonly — no LVQ/LeanVec/scalar-quantized flavours.