[C API] Implement memory estimation functions for index build and search - #364
[C API] Implement memory estimation functions for index build and search#364rfsaliev wants to merge 12 commits into
Conversation
|
This PR contains number of workarounds/TODOs which should be resolved out-of the |
cc12653 to
1d381ec
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Search estimates diverge from actual allocation paths, and the new versioned output APIs lack compatible field handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds C API memory estimation for static/dynamic Vamana index builds and searches.
Changes:
- Adds memory-estimation APIs and storage-specific estimators.
- Adds search-buffer footprint accounting and index integration.
- Adds build wiring and C API tests.
File summaries
| File | Description |
|---|---|
include/svs/index/vamana/search_buffer.h |
Adds static search-buffer estimation. |
include/svs/index/vamana/index.h |
Aligns search allocation and memory accounting. |
include/svs/index/vamana/dynamic_search_buffer.h |
Adds mutable-buffer estimation. |
include/svs/index/vamana/dynamic_index.h |
Updates dynamic search allocation/accounting. |
include/svs/core/data/simple.h |
Exposes blocked-size computation. |
include/svs/core/data.h |
Exposes dataset allocation accounting. |
bindings/c/tests/c_api_test_utils.h |
Adjusts storage-support checks. |
bindings/c/tests/c_api_index.cpp |
Tests static build/search estimates. |
bindings/c/tests/c_api_dynamic_index.cpp |
Tests dynamic estimates. |
bindings/c/src/types_support.hpp |
Adds blocked-allocation size adjustment. |
bindings/c/src/svs_c.cpp |
Implements new C API endpoints. |
bindings/c/src/index_builder.hpp |
Coordinates build/search estimation. |
bindings/c/src/dispatcher_vamana.hpp |
Declares static estimate dispatch. |
bindings/c/src/dispatcher_vamana.cpp |
Implements static estimate dispatch. |
bindings/c/src/dispatcher_dynamic_vamana.hpp |
Declares dynamic estimate dispatch. |
bindings/c/src/dispatcher_dynamic_vamana.cpp |
Implements dynamic estimate dispatch. |
bindings/c/src/data_builder/sq.hpp |
Estimates scalar-quantized storage. |
bindings/c/src/data_builder/simple.hpp |
Estimates simple storage. |
bindings/c/src/data_builder/lvq.hpp |
Estimates LVQ storage. |
bindings/c/src/data_builder/leanvec.hpp |
Estimates LeanVec storage. |
bindings/c/src/data_builder.hpp |
Declares data-size dispatch APIs. |
bindings/c/src/data_builder.cpp |
Implements data-size dispatch. |
bindings/c/include/svs/c/svs_c.h |
Publishes estimation APIs. |
bindings/c/CMakeLists.txt |
Builds the new implementation. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 8
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
klmckeig
left a comment
There was a problem hiding this comment.
Looks good from integration stand of point.
| ResidualBits, | ||
| svs::Dynamic, | ||
| svs::quantization::lvq::Sequential, | ||
| AutoStrategy<PrimaryBits, ResidualBits>, |
There was a problem hiding this comment.
This affects build() too, not just the estimate. A quick local test shows memory goes up for 4-bit LVQ at dims that aren't a multiple of 128 — 105 → 137 B/vector at dim 192 (+30%); 768 and 1536 are unchanged.
using namespace svs::quantization::lvq;
static_assert(Sequential::compute_bytes(4, 192) == 96);
static_assert(Turbo<16, 8>::compute_bytes(4, 192) == 128); // +32 B/vector
static_assert(Sequential::compute_bytes(4, 768) == Turbo<16, 8>::compute_bytes(4, 768));Maybe we could use a template parameter instead, put last so existing LVQDataBuilder<P, R, Alloc> uses still compile?
template <
size_t PrimaryBits,
size_t ResidualBits,
typename Allocator = svs::lib::Allocator<std::byte>,
typename Strategy = svs::quantization::lvq::Sequential>
class LVQDataBuilder {
public:
using data_type = svs::quantization::lvq::LVQDataset<
PrimaryBits,
ResidualBits,
svs::Dynamic,
Strategy,
Allocator>;There was a problem hiding this comment.
Using Sequential strategy for all cases by default conflicts with actual strategy selection in svs::leanvec::detail::PickContainer.
Prefer to follow existing strategy selection in this wrapper for LVQ dataset rather than rewrite the LVQ implementation which can negatively affect performance.
There was a problem hiding this comment.
🟡 Changes recommended
The estimators currently have ABI-safety issues, omit material allocations, and undercount filtered-search memory in supported cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
bindings/c/src/data_builder/leanvec.hpp:109
- These two heap-allocated projection matrices are omitted even though the public API reports estimated memory usage. Their
2 * dimension * leanvec_dims * sizeof(float)footprint can dominate the compressed vectors, especially for small indexes, so the estimate can be substantially low. Include these allocations and aligndataset_allocated_bytes/actual breakdown accounting accordingly.
bindings/c/src/data_builder/lvq.hpp:125 - The LVQ centroid is heap-backed index data, but it is forced to zero here. For small indexes or high-dimensional vectors this makes the advertised total materially low; comparing against the current generic breakdown only hides the same omission there. Account for the centroid in both the estimate and runtime breakdown.
bindings/c/tests/c_api_index.cpp:916 - The exact-equality assertion makes the following 1% tolerance ineffective and contradicts the stated allowance for compressed-storage overhead. Remove this assertion so the intended tolerance governs data-memory comparisons.
bindings/c/src/svs_c.cpp:479
- This check accepts an undersized caller struct, but the function then unconditionally writes every field below. That violates the documented
struct_sizecontract and can write past an older/smaller C struct. Reject undersized layouts or gate each field write by its covered offset, consistently withsvs_index_get_memory_breakdown.
INVALID_ARGUMENT_IF(
out_breakdown->struct_size > sizeof(svs_memory_breakdown_t),
"Incompatible svs_memory_breakdown_t struct_size"
);
bindings/c/src/svs_c.cpp:532
- This check accepts an undersized caller struct, but the unconditional field writes below can then overrun it. Please reject undersized layouts or write only fields covered by
struct_size, as required by the public forward-compatibility contract.
INVALID_ARGUMENT_IF(
out_breakdown->struct_size > sizeof(svs_memory_breakdown_t),
"Incompatible svs_memory_breakdown_t struct_size"
);
bindings/c/src/index_builder.hpp:240
- This assignment discards the configured search-buffer size. For example, with window 1,000, K=10, and filter rate 1.0, the actual iterator starts with the 1,000-candidate hint plus its extra capacity, while this estimates only
extra + 10. Retain the larger configured/candidate requirement.
auto batch_iterator_overhead = svs::ITERATOR_EXTRA_BUFFER_CAPACITY_DEFAULT;
buffer_size = batch_iterator_overhead + num_candidates_needed;
- Files reviewed: 26/26 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Filtered-search estimation can undercount allocations and invoke undefined behavior for extremely small filter rates.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Filtered-search estimates undercount neighbor storage and reject valid searches through an undocumented heuristic limit.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Balanced
d6e5e06 to
8e6652a
Compare
…namic index memory breakdown
…rove search buffer handling
…ltering in search operations
8e6652a to
096b8ae
Compare
This pull request introduces a set of enhancements to the C API and internal implementation to support memory usage estimation for index building and search operations. The primary focus is on adding new estimation functions, implementing the underlying logic for various data builders, and integrating these features with both static and dynamic Vamana index types. The changes also include updates to the build system to incorporate new source files.
New C API for Memory Estimation:
Implementation of Estimation Logic:
estimate_sizemethods for all major data builders (SimpleDataBuilder,LVQDataBuilder,LeanVecDataBuilder,SQDataBuilder) to provide accurate memory usage estimation based on builder configuration, number of vectors, and dimension. [1] [2] [3] [4]data_builder.cppand corresponding header to dispatch and specialize estimation logic for different data types and block sizes. [1] [2]Integration with Vamana Index Types:
Build System and Dependency Updates:
CMakeLists.txtto include the newdata_builder.cppsource file, ensuring the new logic is compiled and linked.Summary of Most Important Changes:
1. API Additions
2. Memory Estimation Logic
estimate_sizemethods for all major data builders to enable accurate memory usage estimation. [1] [2] [3] [4]data_builder.cppand header to centralize and dispatch estimation logic for various data types/blocking strategies. [1] [2]3. Vamana Index Integration
4. Build and Dependency Updates