Add pluggable feature storage and port soft-delete to the runtime index - #2
Merged
Merged
Conversation
Two gaps, both closed on both indexes. ## Pluggable feature storage Upstream rust-cv#39 proposed keeping features outside the heap so a large index does not have to hold its whole corpus in memory. The idea is right; that implementation is not adoptable here. - Its DiskFeatureStore::get returns a reference into a single thread_local! UnsafeCell scratch buffer. Two live references from one store alias the same address. That is UB, and it silently turns distance(a, b) into distance(x, x). - Its MmapFeatureStore::get has no bounds check, so index == capacity dereferences one past the end of the mapping. - Its own tests build 8 points at M=12/M0=24, which never saturates a neighbor list and so never reaches the pruning branch at all - the one place the aliasing bug is fatal. - Its add_neighbor hunk is written against nearest-M truncation. Accepting it would revert d7b9d27 and restore the cluster-isolation bug. - Its copy_from_slice fix is already here, in both indexes, via 8d37b6b. So the trait is written here rather than taken. FeatureStore<T> is a plain borrow-based trait: get_feature returns &T, push_feature appends, feature_count reports the size. Hnsw and HnswRuntime both gain a trailing storage parameter defaulting to Vec<T>, so every existing signature is unchanged. The borrow-stability contract is load-bearing, not decorative. The diversity heuristic added in d7b9d27 holds three feature references live at once while pruning a saturated list - the target, the candidate under consideration, and each already-kept neighbor it is compared against. A store that decoded into a shared buffer would alias all three onto one address, every comparison would collapse toward distance(x, x), the heuristic would degenerate back into nearest-M truncation, and dense clusters would close themselves off again. That is precisely the failure d7b9d27 fixed, so the contract is stated explicitly on the trait and tested rather than assumed. Suitable backings: Vec<T>, a fixed mmap region, an arena, an append-only cache. Unsuitable: reused decode buffers, evicting caches, values synthesized per read. The methods are named get_feature / push_feature / feature_count rather than get / push / len for a concrete reason: a trait method named `get` on Vec<T> shadows slice::get at every call site where the trait is in scope, because trait methods on the receiver type are considered before inherent methods reached through Deref. That broke Vec<bool> indexing in the soft-delete code below. tests/feature_store.rs drives a real mmap-backed store over a real file against a 160-point four-cluster corpus, which does saturate M0=24 lists and does reach the pruning branch: - mmap and Vec storage return identical ranked results, const index - mmap and Vec storage return identical ranked results, runtime index - a dense 37-point far cluster still cannot hide the three points beside the origin when the features live in an mmap - three concurrently live handles keep distinct stable addresses, and a real distance between two of them stays non-zero That last one is the direct test of the contract: against a scratch-buffer store it fails. ## Soft-delete on the runtime index HnswRuntime had no delete surface at all, so the two indexes were not actually mirrors. Ported live_count, is_deleted, mark_delete and entry from the const index, along with both search-path changes: the zero-layer tombstone branch that traverses a deleted node without admitting it to the result heap, and the retain that drops tombstones from the final set. Traversing through tombstones rather than around them is what keeps the graph connected after deletion, and skipping only the result heap is what stops a deleted node from consuming result budget. tests/incremental_delete.rs now covers both indexes, including runtime_and_const_agree_under_identical_deletions, which applies the same deletion set to the same corpus in both and asserts the ranked lists match. ## Verification 21 tests pass with --all-features: clustered_recall 5, feature_store 4, incremental_delete 3, random 2, runtime_parity 1, serde 1, simple 3, simple_discrete 2. runtime_parity and serde passing matter most - the first shows the two indexes did not drift, the second shows the added serde bounds and the skipped PhantomData leave the snapshot format unchanged. cargo clippy --all-targets -- -D warnings is clean with default features and with --all-features, and cargo fmt --check is clean. That required fixing lints that predate this change in hnsw_const.rs, the examples and the benches; no warning was suppressed.
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.
Closes the two gaps left after the cluster-pruning fix (#1), both on both indexes.
Pluggable feature storage
Upstream rust-cv/hnsw#39 proposed keeping features outside the heap. The idea is right; that implementation is not adoptable here:
DiskFeatureStore::getreturns a reference into a singlethread_local! UnsafeCellscratch buffer. Two live references from one store alias the same address - UB, and it silently turnsdistance(a, b)intodistance(x, x).MmapFeatureStore::gethas no bounds check, soindex == capacitydereferences one past the end of the mapping.add_neighborhunk is written against nearest-M truncation; accepting it would revert d7b9d27 and restore the cluster-isolation bug.copy_from_slicefix is already here, in both indexes, via 8d37b6b.So the trait is written here rather than taken.
FeatureStore<T>is borrow-based:get_featurereturns&T,push_featureappends,feature_countreports the size.HnswandHnswRuntimegain a trailing storage parameter defaulting toVec<T>, so every existing signature is unchanged.The borrow-stability contract is load-bearing. The diversity heuristic from #1 holds three feature references live at once while pruning a saturated list - the target, the candidate, and each already-kept neighbor. A store decoding into a shared buffer would alias all three onto one address, every comparison would collapse toward
distance(x, x), the heuristic would degenerate back into nearest-M truncation, and dense clusters would close themselves off again. That is exactly the failure #1 fixed, so the contract is documented on the trait and tested.Methods are named
get_feature/push_feature/feature_count, notget/push/len: a trait method namedgetonVec<T>shadowsslice::getwherever the trait is in scope, because trait methods on the receiver type beat inherent methods reached throughDeref.tests/feature_store.rsdrives a real mmap-backed store over a real file against a 160-point four-cluster corpus that does saturate M0=24 lists:mmap_store_matches_vec_store_on_saturated_clustered_dataruntime_mmap_store_matches_vec_store_on_saturated_clustered_datammap_store_preserves_cross_cluster_reachabilityconcurrently_live_feature_handles_stay_validThe last one fails against a scratch-buffer store - it is the direct test of the contract.
Soft-delete on the runtime index
HnswRuntimehad no delete surface, so the two indexes were not actually mirrors. Portedlive_count,is_deleted,mark_delete,entry, plus both search-path changes: the zero-layer tombstone branch that traverses a deleted node without admitting it to the result heap, and the retain that drops tombstones from the final set.tests/incremental_delete.rsnow covers both, includingruntime_and_const_agree_under_identical_deletions, which applies the same deletion set to the same corpus in both indexes and asserts the ranked lists match.Verification
21/21 tests pass with
--all-features:clustered_recallfeature_store(new)incremental_deleterandomruntime_parityserdesimplesimple_discreteruntime_parityandserdematter most: the first shows the two indexes did not drift, the second shows the added serde bounds and the#[serde(skip)]PhantomDataleave the snapshot format unchanged.cargo clippy --all-targets -- -D warningsis clean with default features and with--all-features;cargo fmt --checkis clean. That required fixing lints predating this change inhnsw_const.rs, the examples and the benches. No warning was suppressed.