Make build RAM estimation store-owned - #1277
Open
wuw92 wants to merge 2 commits into
Open
Conversation
`estimate_build_index_ram_usage` hand-wrote a byte formula with no link to the data structures it was describing, so layout changes silently invalidated it. It had already drifted: the graph term applied `GRAPH_SLACK_FACTOR` on top of a `max_degree` that already includes slack, and it ignored the per-row length prefix, 64-byte alignment padding, frozen points, and the per-point lock vectors. Each store now exposes a sizing helper that its own constructor consumes, so allocation and estimation cannot diverge: - `AlignedMemoryVectorStore::layout` is the single source of truth for the backing allocation; `with_capacity` and `estimate_bytes` both derive from it. - `SimpleNeighborProviderAsync`, `FastMemoryVectorProviderAsync`, `FastMemoryQuantVectorProviderAsync`, and `SQStore` share their row-width and lock-count computations between `new` and `estimate_bytes`. - A narrow `EstimateBytes` trait is implemented for the precursors disk build actually uses, and `DefaultProviderParameters::estimated_bytes` aggregates them using the same `npts` and start-point constants as `new_empty`. The estimator and the `provider_parameters` helper move to `inmem_builder.rs`, next to `new_inmem_index_builder`, whose `match` over `BuildQuantizer` the estimator must mirror. This also removes `build.rs`'s dependency on `merged_index.rs`, which existed only to borrow the estimator. The partition-time estimate sizes against the shard graph config it now shares with `create_shard_index_config`. This is an estimate, not an accounting: allocations that scale with the point count are counted, fixed overheads such as PQ pivots and quantizer parameters are not. Build strategy thresholds shift accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 482d6098-133f-48d7-8c5b-317dae385fe7
Slack is already resolved when the index configuration is built: config::MaxDegree::Slack folds the factor into the final max_degree, and DefaultProviderParameters::max_degree is documented as the actual maximum number of neighbors. Every call site therefore passed 1.0. Keeping the parameter is what let the estimate and the allocation drift apart in the first place, and applying slack twice would silently over-allocate each adjacency list without tripping the length assert in set_neighbors_sync. Dropping it makes new and estimate_bytes agree by construction rather than by both being handed the same constant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4d98f122-fb99-47d3-88f9-9d257a55c8c8
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
1 participant
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.
Make build RAM estimation store-owned
Stacked on #1271 — review against
wuw92/refactor-disk-build-structure-main.Why
Addresses #1271 (comment).
The build RAM estimate was a hand-written byte formula with no link to the stores it described, so layout changes silently invalidated it.
What
Make each store answer for its own size, so an allocation and its estimate cannot diverge. Every estimate mirrors one constructor at its own level and knows only what that constructor directly builds:
The estimator moves beside the in-memory builder.
Scoped to the stores disk build uses; small or fixed overheads such as PQ pivots stay uncounted.
Also removes the
graph_slack_factorparameter fromSimpleNeighborProviderAsync: slack is already folded intomax_degree, so every call site passed1.0.