separate seqlock fine grain into separate index by ethanglaser · Pull Request #369 · intel/ScalableVectorSearch

ethanglaser · 2026-08-20T23:51:32Z

main...razdoburdin:ScalableVectorSearch:seqlock without modifying existing index

What this PR does

Reproduces the functionality of razdoburdin:ScalableVectorSearch:seqlock — lock-free search concurrent with add_points, delete_entries, and consolidate — 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 in svs::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 `seqlock`

For every ported file, namespace names, include paths, concurrent:: qualification, comments, and blank lines were normalized away, then diffed against the seqlock version. 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

File (lines)	Counterpart in `seqlock`	Diff vs `seqlock`	Reason for the diff
`lib/segmented_vector.h` (330)	same path (+330)	byte-identical	—
`lib/concurrency/seqlock.h` (141)	same path (+141)	byte-identical	—
`lib/concurrency/atomic_span.h` (93)	same path (+93)	byte-identical	—
`concurrent/consolidate.h` (525)	`index/vamana/consolidate.h` (+239)	0 lines	namespace only
`concurrent/multi.h` (967)	`index/vamana/multi.h` (+124)	0 lines	namespace only
`concurrent/prune.h` (338)	`index/vamana/prune.h` (+89)	0 lines	namespace only
`concurrent/vamana_build.h` (605)	`index/vamana/vamana_build.h` (+32)	0 lines	namespace only
`concurrent/dynamic_search_buffer.h` (625)	`index/vamana/dynamic_search_buffer.h` (+8)	0 lines	namespace only. 98% identical to upstream — carried whole because the namespace needs its own `MutableBuffer`
`concurrent/graph_concepts.h` (222)	`concepts/graph.h` (+22)	0 lines	namespace only (`AddEdgeResult` and the `MemoryGraph` concept are identical)
`concurrent/translation.h` (491)	`core/translation.h` (+77)	1 line	the `namespace` declaration
`concurrent/reverse_edges.h` (99)	`core/graph/reverse_edges.h` (+99)	3 lines	namespace + include path
`concurrent/iterator.h` (358)	`index/vamana/iterator.h` (+117)	2 lines	uses the non-locking `unsafe_translate_internal_id` under the caller's lock — `std::shared_mutex` is not recursive, and the source branch re-acquires it per neighbor
`concurrent/graph.h` (677)	`core/graph/graph.h` (+240)	59 lines	(a) the 10 `std::atomic_ref` adjacency-slot accesses route through `relaxed_load`/`relaxed_store` — same relaxed ops, one gate for the TSan negative control; (b) `SegmentedBlockedData` in place of the modified `BlockedData`; (c) `rebuild_reverse_edges` records unconditionally (fix 1 below)
`concurrent/greedy_search.h` (203)	`index/vamana/greedy_search.h` (+85)	62 lines	5 `using`-declarations replace verbatim copies of `NullTracker`, `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 identical
`concurrent/dynamic_index.h` (2080)	`index/vamana/dynamic_index.h` (+681)	85 lines	(a) two-flavour translator accessors, `foo` / `unsafe_foo` (fix 2); (b) `supplement_search_buffer` + a `single_search` `svs_invoke` override in place of the source branch's edit to `index/vamana/extensions.h`; (c) 3 qualification lines
`concurrent/blocked_data.h` (433)	`core/data/simple.h` (+13)	new specialization	Same `std::vector` → `lib::SegmentedVector` block-directory swap the source branch applies to `SimpleData<T, Extent, Blocked<Alloc>>`, applied instead to a new `SegmentedBlocked<Alloc>` allocator tag, so no existing dataset type changes. Result is still a `SimpleData`, so the dataset concepts, `extensions`, `compact_data`, and the serializer all apply unchanged. Also adds release/acquire on `size_` (fix 3)
`concurrent/spinlock.h` (49)	`lib/spinlock.h` (+7)	subclass	Identical four value-initializing copy/move operations (`SegmentedVector<SpinLock>` needs them), added by subclassing rather than by editing `lib/spinlock.h`
`concurrent/concurrent.h` (54)	none	new	Umbrella header + the `svs::concurrent` namespace alias. The source branch needs no analogue because it edits upstream in place
`concurrent/README.md`	none	new	Design doc: concurrency model, lock ordering, slot lifecycle, divergence list, TSan instructions

Tests and build

File (lines)	Counterpart in `seqlock`	Diff vs `seqlock`	Reason for the diff
`tests/svs/lib/segmented_vector.cpp` (237)	same path (+237)	byte-identical	—
`tests/svs/concurrent/translation.cpp` (288)	`tests/svs/core/translation.cpp` (+40)	scaffolding only	`[concurrent]` tags, type aliases, `SegmentedBlockedData` for `BlockedData`, and a local data loader so `tests/utils/` needs no edit
`tests/svs/concurrent/dynamic_index.cpp` (548)	`tests/svs/index/vamana/dynamic_index.cpp` (+108)	scaffolding + save/load path	Same, plus save/load round-trips through `auto_dynamic_assemble` instead of `svs::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)	none	ports of the upstream Vamana/graph tests, re-pointed at the new index	The source branch needs no new test files because it modifies the index the existing tests already exercise. `prune.cpp` asserts the new `reenable` semantics (fix 4); `dynamic_index_2.cpp` additionally scopes its helper block in an anonymous namespace (ODR: the upstream file declares the same `Report` and its stream operator) and scopes its global-logger sink push with a `ScopedGlobalSink` guard (the upstream version leaks a sink that outlives, by reference, the vector it captures)
`tests/svs/concurrent/concurrency.cpp` (578)	none	new	Searches running against the index while writers insert, delete, and consolidate; checks id round-trips, result-set consistency, and post-mutation recall. The source branch has no multi-threaded test
`tests/CMakeLists.txt` (+75)	same path (+1)	registers the 9 new test files, plus two opt-in ThreadSanitizer targets	See ThreadSanitizer below

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_edges records every edge. seqlock skips edges whose reverse also exists, halving the index on the reasoning that gather_work_set visits out(d) ∪ R(d). That invariant holds right after a rebuild but is not maintainable: the moment consolidation rewires d and drops d -> u, u's in-edge becomes invisible, and a later deletion of d leaves u pointing at a retired slot. It is masked in seqlock because every one of its tests pairs consolidate() with compact(), which rebuilds from scratch. Repeated consolidate() without compact() — the cheap maintenance path — corrupts the graph within two rounds (debug_check_invariants() reports Node number N has an invalid (Empty) neighbor). Recording unconditionally gives the strictly stronger R(d) ⊇ in(d), which no edge removal can break.
Translator reads are synchronized. translate_external_id, translate_external_id_or, has_id, translate_internal_id, and on_ids read the tsl::robin_maps with no lock in seqlock while add_points inserts and consolidate erases under translator_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.
Dataset size_ is published with release/acquire. It is written by resize() while lock-free searches read it via size(); both are plain accesses in seqlock. TSan reports it on any search overlapping an add_points.
PruneState test expectations. seqlock redefines reenable to promote Candidate rather than Pruned back to Available, but leaves the pre-existing tests/svs/index/vamana/prune.cpp asserting reenable(Pruned) == Available. Here the concurrent PruneState is a distinct type in a distinct namespace, so the upstream test keeps passing against upstream reenable, and the ported test asserts the new semantics.

Three changes in seqlock are deliberately not ported, none of which affect index behaviour: NullLockGuard in index/vamana/index.h (only needed because seqlock shares one BatchIterator with the static index; this stack has its own), capacity() in quantization/scalar/scalar.h (memory reporting, unused here), and the Python binding/GIL changes.

Testing


`ctest -L "concurrent\|segmented_vector"`	24/24 pass (225,377 assertions)
Concurrency test	2 writer threads inserting disjoint halves in 500-point batches with interleaved deletes, 8 concurrent searchers, ~15,000 searches; then `debug_check_invariants()`, post-mutation recall > 0.85, `consolidate()` + `compact()`, re-check
Rest of the suite	unaffected — no pre-existing header is modified

ThreadSanitizer

Correctness here rests almost entirely on memory ordering, which an uninstrumented test can only fail to disprove:

cmake -DSVS_EXPERIMENTAL_ENABLE_CONCURRENT_TSAN=YES ...
ctest -L tsan

concurrent_tsan must come out clean (it does — 0 warnings). concurrent_tsan_negative is a negative control, registered WILL_FAIL TRUE: it defines SVS_CONCURRENT_UNSAFE_PLAIN_GRAPH_ACCESS, degrading the adjacency-slot accessors to plain loads and stores, and reports 54 races, all in relaxed_load/relaxed_store. A clean positive run only means something if the same run fires once the atomics are removed.

Scope

Library level only. Nothing outside include/svs/concurrent/ references the new index: no svs::DynamicVamana orchestrator support, no Python bindings, no benchmark integration. With seqlock, 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.
Not yet measured. No throughput or latency comparison against seqlock or against the existing dynamic index.
Build coverage so far: GCC 11.4, x86-64 Linux. Not yet exercised on GCC 12, Clang 15/18, macOS, or ARM. Two notes for CI: std::atomic_ref is absent from libc++ before LLVM 19 (seqlock has the same exposure), and every concurrency claim so far is validated on x86's strong memory model.
Coverage shape: all multi-threaded testing is in concurrency.cpp; MultiMutableVamanaIndex is ported but has no concurrent test. Dataset coverage is float32 SegmentedBlockedData only — no LVQ/LeanVec/scalar-quantized flavours.

ahuber21

Before I approve I have some small nitpicks / confusions that I'd like to discuss. See code comments.

Generally, I will be approving this. But only because we're trying to meet a deadline and this doesn't interfere with existing code.
I don't like the gigantic duplication and we have to refactor the new concurrency feature completely. I'm exploring some ideas. But really, this can't stay for long.

ahuber21 · 2026-08-31T13:41:54Z

+    // Reserved by an in-flight add_points: slot owned by the adder, vector
+    // copied, adjacency list being built. Invisible to search, consolidate,
+    // and subsequent add_points until promoted to Valid.
+    Pending = 0x04,


Why 0x04 and not 0x03 ?

I planed to use bitwise operation, but haven't done it. So it is a legacy, we can change it to 0x03

That's what I would suggest. Or, if bitwise can still happen down the read, at least use 1 << 0; 1 << 1; 1 << 2; etc. to make the intention more obvious.

ahuber21 · 2026-08-31T14:12:41Z

+
+            // Validate that no concurrent write occurred during the read.
+            if (graph.seq_counters()[node_id].read_validate(*maybe_seq)) {
+                break; // Consistent read — proceed to the next node.


We've modified some buffers above. Should these changes be reverted before the next retry? Or is it okay to have stale data?

My idea here is the following:

We have already done most of the job for this stale node,

This node is a valid node, just not in a neighbor list.

So we can safely use it for search, just need to retry not to miss the new neighbors.

The the data in the buffers is never wrong, maybe just incomplete?

ahuber21 · 2026-08-31T14:16:46Z

+            if (is_deleted(dst)) {
+                // SeqLock retry: a concurrent consolidate may be writing dst's
+                // neighbors if dst is not deleted in the other consolidate's view.
+                for (;;) {


General comment: These can become deadlocks, or is there any max retry logic or a timeout?

In seqlock a reader never blocks a writer, so seqlock doesn't suffer from deadlocks.
It could be a starvation issue, if writer always modifies the same node, and reader should retry again and again. But such case means that we can't use this node for search anyway, since it doesn't have a stable state and is always mutating.

What if a writer crashes or exits out without increasing the sequence counter?

ahuber21 · 2026-08-31T14:17:54Z

+/// lock (e.g., per-node ``SpinLock``) to serialize concurrent writers to the same element.
+///
+class SeqLockCounter {
+    using counter_type = uint8_t;


Would it hurt to use a wider type for some additional safety? Not sure if it's realistic that this counter wraps during a read, but a uint32_t certainly wouldn't.

now it is +1 byte for each node, uint32_t is +4 bytes. I am not very sure if this 3 additional bytes per node are critical, but in my mind it is better to save as much as possible.

The only way the counter overflow would be an issue is the following:
writer modify the data exactly 128 times during a single reader retry.

Taking into account that writers for the same node are serialized, and reading time is close or even smaller than write time, I don't see a realistic way overflow will be a problem.

But if 1 or 3 additional bytes per node are acceptable, we can bump counter to uint16_t of uint32_t.

Yeah, you're right. Maybe @aguerreb can comment if there are ever scenarios where a single node will be hit so hard that the pure coincidence of 256 writes during a single read is by any means likely.

ethanglaser added 2 commits August 20, 2026 16:51

separate seqlock fine grain into separate index

69d990a

minor refactoring for vecsim experimentation

21a7231

ahuber21 reviewed Aug 31, 2026

View reviewed changes

ethanglaser requested a review from razdoburdin August 31, 2026 22:34

ethanglaser added 2 commits September 1, 2026 08:52

attempt to address heap-use-after-free

7e58381

formatting

39c1f39

Uh oh!

Conversation

ethanglaser commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

How the diff was verified against seqlock

Implementation

Tests and build

Intentional functional divergences

Testing

ThreadSanitizer

Scope

Uh oh!

ahuber21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ethanglaser commented Aug 20, 2026 •

edited

Loading

How the diff was verified against `seqlock`