Repository navigation
[C API] Add SVS_SYNC_KIND_FINE_GRAIN support using ConcurrentDynamicVamana - #418
Conversation
6d21b35 to
a31079e
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open findings
BlockKind::NoneandBlockKind::Blockedspecializations under… · NewBlockKind::NoneandBlockKind::Blockedspecializations under… · Newsizeof(std::vector<T>)doesn’t include heap… · Newmin_idto0can permanently expand the sampled ID range when the first inserted… · Newreturnleaksstoragewhenstorage_usable(storage)is false. Please freestorage… · New
What changed in this PR
Adds C API support for SVS_SYNC_KIND_FINE_GRAIN by wiring in the concurrent dynamic Vamana implementation and updating storage-layout selection, memory reporting, and docs to reflect the fine-grained concurrency model.
Changes:
- Introduces concurrent dynamic Vamana dispatch/build/load/copy paths and a
ConcurrentIndexVamanawrapper for C API synchronization semantics. - Generalizes dataset/graph storage layout selection via
BlockKind/MaybeBlockedAllocacross all data builders and dispatchers. - Updates C API docs/headers and tests to cover fine-grained sync behavior and memory accounting (including reverse edges).
| File | Description |
|---|---|
| include/svs/orchestrators/concurrent_dynamic_vamana.h | Adds logger plumbing and stream-load graph type tweaks for concurrent dynamic Vamana. |
| bindings/c/src/index_vamana.hpp / .cpp | Adds ConcurrentIndexVamana and atomic ID-range tracking for concurrent updates. |
| bindings/c/src/index_builder.hpp / .cpp | Selects concurrent vs regular dynamic index implementation; adds sync-kind-aware memory estimation. |
| bindings/c/src/dispatcher_concurrent_vamana.hpp / .cpp | New dispatcher for build/load/copy and memory estimate for concurrent dynamic Vamana. |
| bindings/c/src/dispatcher_dynamic_vamana_copy.hpp | New shared dynamic-index conversion layer (flavors) used by regular/concurrent paths. |
| bindings/c/src/dispatcher_dynamic_vamana.cpp | Refactors copy path to use shared copy dispatcher and BlockKind-based specialization enumeration. |
| bindings/c/src/dispatcher_vamana.cpp | Switches static Vamana dispatcher specialization enumeration to BlockKind. |
| bindings/c/src/allocator.hpp | Introduces BlockKind and MaybeBlockedAlloc to choose None/Blocked/Segmented layouts. |
| bindings/c/src/data_builder*.hpp / data_builder.hpp / data_builder.cpp | Updates specialization enumeration to BlockKind and threads concurrent extension headers. |
| bindings/c/src/svs_c.cpp | Updates memory breakdown reporting to include concurrent reverse edges as graph memory. |
| bindings/c/include/svs/c/svs_c.h | Documents fine-grained sync behavior and memory estimation semantics. |
| bindings/c/docs/C_API_Design.md | Updates public design docs for fine-grain sync, conversion, and reverse-edge memory reporting. |
| bindings/c/tests/c_api_dynamic_index_sync.cpp | Adds tests for sync-kind conversion, fine-grain storage compatibility, and memory reporting. |
| bindings/c/CMakeLists.txt | Adds new dispatcher/copy sources to the C API target. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a31079e to
5b1bd90
Compare
razdoburdin
left a comment
razdoburdin
left a comment
There was a problem hiding this comment.
I have few findings. The most critical one is a serialization of operations by NativeThreadPool being used by default.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
9 open findings
BlockKind::NoneandBlockKind::Blockedspecializations under…BlockKind::NoneandBlockKind::Blockedspecializations under…min_id/max_idto extreme values is unsafe if the index is ever empty or… · Newmin_idto0can permanently expand the sampled ID range when the first inserted…sizeof(std::vector<T>)doesn’t include heap…allocator.hppis already included near the top of this header; re-including it in the `#else //… · Newallocator.hppmultiple times (including in the disabled-feature stub… · Newallocator.hppmultiple times (including in the disabled-feature stub… · Newallocator.hppmultiple times (including in the disabled-feature stub… · New
1 resolved since last review
🧠 Review effort: Lite
a39da58 to
9bbee5f
Compare
| SVS_SYNC_KIND_GLOBAL = 1, ///< Index-wide reader/writer lock | ||
| SVS_SYNC_KIND_FINE_GRAIN = 2 ///< Currently the same as GLOBAL; reserved for | ||
| ///< finer-grained locking | ||
| SVS_SYNC_KIND_FINE_GRAIN = 2 ///< Concurrent index; updates run in parallel |
There was a problem hiding this comment.
An index built with FINE_GRAIN and loaded without sync_kind silently becomes a normal index with no locking. Could we save the sync kind and warn when the load uses a different one? A test for this case would help too.
There was a problem hiding this comment.
@razdoburdin, do you know if there is a method to distiguish between 'basic' and 'concurrent' MutableVamanaIndex versions saved in a filesystem/stream?
I mean adding the sync_kind flag to a saved index at the C API level leads the cost for backward compatibility.
Anyway, it seems like would be better to implement full serialization/deserialization functionality rather than extending saving/loading with more and more index flags and details with a cost of backward compatibility.
There was a problem hiding this comment.
There isn't such way, but you can save non-empty concurrent Index to stream and load it as non-concurrent and vice-versa.
The only problem is empty index. But we can use workaround and create new empty index of the required type.
I suggest to fix it as an invariant and add corresponding tests.
| ); | ||
| auto vamana_algorithm = static_cast<AlgorithmVamana*>(algorithm.get()); | ||
| return dispatch_dynamic_vamana_memory_estimate( | ||
| auto estimate = uses_concurrent_index(sync_kind) |
There was a problem hiding this comment.
Could we apply the same patter for build and load too?
There was a problem hiding this comment.
What do you mean under the "same patter"?
Use a function pointer instead of direct call?
ab771a6 to
9cc1c72
Compare
There was a problem hiding this comment.
There are two remaining issues from my side:
- Non-valid ids under concurrent consolidation [done, filter deleted ids was chosen]
- Writer starvation issue. [done, fix merged into master]
For the first one #429 or #430 should be merged in the PR's branch.
If #429 is chosen as a fix for (1), #424 should be merged in main first to address (2). #430 already contains fix for (2).
…amana - Added atomic tracking for minimum and maximum IDs in DynamicIndexVamana to support concurrent operations. - Introduced ConcurrentIndexVamana to manage concurrent access and operations on the dynamic index. - Updated memory estimation functions to account for new synchronization parameters. - Implemented comprehensive tests for synchronization kind conversion and memory usage. - Created dispatcher for concurrent Vamana index build and load operations. - Added support for copying between concurrent and regular dynamic indices.
* Fix set_num_threads() to handle sync kind. * Apply single-vector limitations for search/add/delete in fine-grain index. * Update tests according to limitations.
1e61991 to
59739c1
Compare
This PR fixes potential writer starvation for compact_mutex by adding a gatekeeper. Address one of the comments from: #418
This pull request adds support for a concurrent dynamic index ("fine-grained" synchronization) in the C API, updating both the implementation and documentation to reflect the new concurrency model. It introduces a generalized way to select storage layouts based on the concurrency mode, and updates various data builder utilities to use this new approach. The documentation and C API headers are also updated to clarify the behavior and memory usage of the new concurrent index.
Concurrent index support and synchronization model:
dispatcher_concurrent_vamana.hpp/cpp,dispatcher_dynamic_vamana_copy.hpp). [1] [2]SVS_SYNC_KIND_FINE_GRAINmode, which uses a concurrent index allowing parallel updates and searches, and clarified the locking behavior and memory reporting for this mode. [1] [2] [3] [4]Generalization of storage layout selection:
BlockKindenum and a generalizedMaybeBlockedAlloctemplate to select between contiguous, blocked, or segmented (concurrent) storage layouts based on the concurrency mode.for_simple_specializations,for_sq_specializations,for_leanvec_specializations,for_lvq_specializations) to use the newBlockKind-based selection instead of a simple boolean for blocked/unblocked storage. [1] [2] [3] [4] [5] [6]Concurrent extension support for data builders: