From d88f471db5c47c8aa7ac5a4df982d49f9cd24885 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 13:49:18 -0600 Subject: [PATCH 01/11] Beam the upper-layer descent so a local minimum cannot strand a query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `greedy_descend` walked the upper layers with a beam of one: it stopped at the first node no neighbour improved on, then dropped a level. On a clustered corpus that local minimum can sit in the wrong basin, and layer-0 adjacency is intra-basin, so the layer-0 beam has no uphill edge with which to leave. The query's true nearest neighbour is then unreachable at any ef, which is what made `concurrent_insert_search`'s `misses == 0` assertion fail ~1.5% of the time (6/400 runs on an oversubscribed loop) and blocked the v0.2.0 release. `beam_descend` runs the existing `search_layer` at width DESCENT_EF=16 on each upper level instead, seeded by the level above's best, rolling the scratch epoch per level so a node present at several levels is expandable at each. It is shared by the read and the write path, as the greedy descent was: insert must route through the same graph its queries will, or nodes get their neighbours chosen from a basin searches never reach. Concurrency was never the cause — it only shuffles the insertion order. Fixing that order reproduces the trap single-threaded, which is what the new regression test pins: seeds 57 and 240 lose 55 and 22 of their 8000 self-queries on the old descent and none on the new one. Measured (8000-node builds, self-query every node at ef 256, one build per seed): width 1 loses 125 nodes over 200 builds, width 4 loses 20 over 200, width 8 loses 8 over 700, width 16 loses 0 over 700. The oversubscribed concurrent loop goes 6/400 failures to 0/400. Cost at 50k x 768-d: visits/query +17-27%, p50 +0.06 ms (0.15 -> 0.21 at ef 16, 0.46 -> 0.47 at ef 512), build throughput -20%; recall@10 improves below ef 128 and is unchanged above. Filtered and predicated searches now count their visit budget from where the descent left off. The budget is documented as the layer-0 cap, and a wider descent would otherwise have silently eaten into it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- DESIGN.md | 23 ++++++++++++++++ src/insert.rs | 15 ++++++++-- src/search.rs | 67 ++++++++++++++++++++++++++++----------------- tests/concurrent.rs | 66 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 28 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 66dff99..44bf070 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -210,6 +210,29 @@ search(sliceHandles, queryVector: Float32Array, k, ef, filter?): Promise<{ids, d - Auto-ef / auto-efC read the node count from the header high-water minus freelist length — same semantics as today, minus the #2182 inflation (freed ids return to the pool). +**Upper-layer descent is a beam, not hill climbing** (`beam_descend`, `DESCENT_EF = 16`). The +textbook width-1 descent halts at the first upper-layer node no neighbor improves on. On a +clustered corpus that local minimum can sit in the wrong basin, and layer-0 adjacency is +intra-basin, so the layer-0 beam has no uphill edge with which to leave — the query's true +nearest neighbor is then unreachable at *any* ef, and raising ef only expands the wrong basin. +Measured on the `tests/concurrent.rs` corpus (8 000 nodes, 64-d, self-query every node at +ef 256, insertion order fixed by seed): width 1 loses 125 nodes over 200 builds, width 4 loses +20 over 200, width 8 loses 8 over 700, width 16 loses 0 over 700. Cost at 50 000 × 768-d: +visits/query +17 % to +27 %, p50 +0.06 ms flat (0.15 → 0.21 ms at ef 16, 0.21 → 0.28 at ef 64, +0.46 → 0.47 at ef 512 — the descent is a fixed cost, so it hurts most where ef is small), build +throughput -20 %. recall@10 improves below ef 128 (0.844 → 0.903 at ef 16, 0.983 → 1.000 at +ef 64) and is unchanged above. + +`beam_descend` is shared by the read and write paths deliberately: insert must route through +the same graph its queries will, or nodes get their neighbors chosen from a basin searches +never reach. The width is a compile-time constant rather than a parameter because it is a +correctness floor, not a recall/latency dial — `ef` is the dial. + +Two facts worth keeping when working on this. The trap is a property of graph *shape*, not of +concurrency: it reproduces single-threaded from a fixed insertion permutation, and concurrency +only shuffles that permutation. And it needs the full corpus — no seed reproduces it at 32 +dims, or at 2 000 / 4 000 nodes, so a shrunken repro is not evidence of a fix. + **Filtering** (predicate-aware / ACORN, `filteredSearch = true` today): 1. **Bitset fast path.** RBAC allow-lists and companion-condition candidate sets are computed diff --git a/src/insert.rs b/src/insert.rs index f20f84e..20b948d 100644 --- a/src/insert.rs +++ b/src/insert.rs @@ -7,7 +7,7 @@ use crate::distance::{quantize_int8, Query}; use crate::format::{NO_ID, NO_UPPER}; use crate::graph::Graph; -use crate::search::{greedy_descend, search_layer, SearchScratch, SearchStats}; +use crate::search::{beam_descend, search_layer, SearchScratch, SearchStats, DESCENT_EF}; pub struct InsertParams { pub m: usize, // base connection count (JS M, default 16) @@ -229,8 +229,17 @@ pub fn insert( return Err(InsertError::Wedged); }; let top = level.min(entry_level as u8); - let (mut ep, mut ep_dist) = - greedy_descend(graph, &query, entry_id, entry_dist, entry_level, top as u32, &mut stats); + let (mut ep, mut ep_dist) = beam_descend( + graph, + &query, + entry_id, + entry_dist, + entry_level, + top as u32, + DESCENT_EF, + scratch, + &mut stats, + ); // Per-level connection lists for the new node, selection-ordered. let mut connections: Vec> = vec![Vec::new(); level as usize + 1]; diff --git a/src/search.rs b/src/search.rs index 9e8ca8b..fe62351 100644 --- a/src/search.rs +++ b/src/search.rs @@ -3,7 +3,7 @@ //! array; neighbor ids stream through a reusable scratch buffer. use crate::distance::Query; -use crate::format::NO_ID; +use crate::format::{MAX_UPPER_LEVELS, NO_ID}; use crate::graph::Graph; use std::cmp::Ordering as CmpOrdering; use std::collections::BinaryHeap; @@ -184,35 +184,40 @@ pub fn search_layer( out } -/// Greedy single-candidate descent through upper layers from `from_level` down to -/// `to_level` (exclusive lower bound handled by caller loops). Returns improved entry. -pub fn greedy_descend( +/// Beam width carried through every upper level of the descent. +/// +/// A width-1 descent is plain hill climbing: it halts at the first node no neighbor improves +/// on. On a clustered corpus that local minimum can sit in the wrong basin, and layer-0 +/// adjacency is intra-basin, so the layer-0 beam has no uphill edge to leave it — the query's +/// true neighborhood is then unreachable at any ef. +pub const DESCENT_EF: usize = 16; + +/// Beam descent through upper layers from `from_level` down to `to_level` (exclusive). +/// Each level runs a width-`ef` beam seeded by the level above's best; that level's best in +/// turn seeds the next. Returns the improved entry for the caller's layer-`to_level` search. +/// +/// The scratch epoch is rolled per level: a node reachable at several levels must be +/// expandable at each of them, so visited marks must not carry across the boundary. +pub fn beam_descend( graph: &Graph, query: &Query, mut current: u32, mut current_dist: f32, from_level: u32, to_level: u32, + ef: usize, + scratch: &mut SearchScratch, stats: &mut SearchStats, ) -> (u32, f32) { - let mut nbuf: Vec = Vec::new(); - let mut level = from_level; + let mut level = from_level.min(MAX_UPPER_LEVELS as u32); while level > to_level { - let mut improved = true; - while improved { - improved = false; - graph.upper_neighbors_into(current, level.min(255) as u8, &mut nbuf); - for i in 0..nbuf.len() { - let nid = nbuf[i]; - if let Some(d) = graph.distance_to(nid, query) { - stats.visits += 1; - if d < current_dist { - current = nid; - current_dist = d; - improved = true; - } - } - } + scratch.begin(graph.file.id_high_water()); + let found = + search_layer(graph, query, current, current_dist, ef, level as u8, scratch, stats, None, u64::MAX); + // search_layer admits the entry itself, so `first` is never worse than the entry + if let Some(&(id, d)) = found.first() { + current = id; + current_dist = d; } level -= 1; } @@ -267,8 +272,9 @@ pub fn search( let Some((entry_id, entry_level, entry_dist)) = resolve_entry(graph, query, &mut stats) else { return (Vec::new(), stats); }; + let (ep, ep_dist) = + beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin(graph.file.id_high_water()); - let (ep, ep_dist) = greedy_descend(graph, query, entry_id, entry_dist, entry_level, 0, &mut stats); let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX); out.truncate(k); (out, stats) @@ -289,9 +295,16 @@ pub fn search_filtered( let Some((entry_id, entry_level, entry_dist)) = resolve_entry(graph, query, &mut stats) else { return (Vec::new(), stats); }; + let (ep, ep_dist) = + beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - let (ep, ep_dist) = greedy_descend(graph, query, entry_id, entry_dist, entry_level, 0, &mut stats); - let budget = if filter.is_some() { (ef * filter_expansion) as u64 } else { u64::MAX }; + // offset by the descent's own visits so the budget bounds layer 0, as documented, rather + // than layer 0 minus whatever the descent already spent + let budget = if filter.is_some() { + stats.visits.saturating_add((ef * filter_expansion) as u64) + } else { + u64::MAX + }; let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, filter, budget); out.truncate(k); (out, stats) @@ -332,8 +345,12 @@ pub fn search_predicated( let Some((entry_id, entry_level, entry_dist)) = resolve_entry(graph, query, &mut stats) else { return (Vec::new(), stats); }; + let (ep, ep_dist) = + beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - let (ep, ep_dist) = greedy_descend(graph, query, entry_id, entry_dist, entry_level, 0, &mut stats); + // absolute cap for the layer-0 loop below: the caller's budget counted from where the + // descent left off, so a wider descent does not silently shrink it + let visit_budget = stats.visits.saturating_add(visit_budget); use std::collections::HashMap; let mut verdicts: HashMap = HashMap::new(); diff --git a/tests/concurrent.rs b/tests/concurrent.rs index e3699e2..09b0607 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -1,5 +1,7 @@ //! Concurrent-write torture: writers insert while readers search; then verify the graph is //! coherent (every stored vector findable, edge lists within cap, freelist reuse works). +//! `vector_for`'s clustered corpus is also what makes the deterministic descent regression at +//! the bottom of this file bite, so the two live together rather than duplicating it. use hnsw_plane::distance::Query; use hnsw_plane::insert::{insert, InsertParams}; @@ -172,3 +174,67 @@ fn racing_first_inserts_all_stay_reachable() { let _ = std::fs::remove_file(&path); } } + +/// A reproducible insertion order over the corpus: Fisher-Yates driven by a xorshift stream, so +/// one seed names one exact graph with no thread interleaving in it. +fn insertion_order(n: u32, seed: u64) -> Vec { + let mut order: Vec = (0..n).collect(); + let mut s = seed.wrapping_mul(0x9e37_79b9_7f4a_7c15) | 1; + for i in (1..order.len()).rev() { + s ^= s << 13; + s ^= s >> 7; + s ^= s << 17; + order.swap(i, (s % (i as u64 + 1)) as usize); + } + order +} + +/// The deterministic half of `concurrent_insert_search`'s reachability assertion, and the +/// regression pin for the upper-layer descent. +/// +/// Concurrency is not what breaks the search here — it only shuffles the insertion order, which +/// this test fixes outright. A width-1 greedy descent halts at the first upper-layer node no +/// neighbor improves on; on this corpus that local minimum can sit in the wrong basin, and +/// layer-0 adjacency is intra-basin, so the layer-0 beam has no uphill edge with which to leave. +/// The query's own vector is then unreachable at any ef, which is what the sampled assertion +/// above catches only ~1.5% of the time. Both seeds trap `greedy_descend`: seed 57 loses 55 of +/// its 8000 self-queries and seed 240 loses 22. +#[test] +fn a_descent_that_traps_at_a_local_minimum_still_reaches_the_true_neighborhood() { + let dims = 64; + let n = 8_000u32; + for &seed in &[57u64, 240] { + let path = std::env::temp_dir().join(format!("hnsw-descent-{}-{seed}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 32, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + + let inserted: Vec<(u32, u32)> = insertion_order(n, seed) + .into_iter() + .map(|index| { + let v = vector_for(index, dims); + (index, insert(&graph, &v, ¶ms, &mut scratch).expect("insert")) + }) + .collect(); + + let mut misses = Vec::new(); + for &(index, id) in &inserted { + let query = Query::new(vector_for(index, dims)); + let (results, _) = search(&graph, &query, 10, 256, &mut scratch); + if !results.iter().any(|&(rid, _)| rid == id) { + misses.push((index, id)); + } + } + assert!( + misses.is_empty(), + "seed {seed}: {} of {n} nodes are their own true nearest neighbor but unreachable \ +from the entry point — the descent stranded the search. First few (corpus index, node id): {:?}", + misses.len(), + &misses[..misses.len().min(8)] + ); + + drop(graph); + let _ = std::fs::remove_file(&path); + } +} From 315b39bf4b5699473e9613dc5914371342de02c9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 13:50:34 -0600 Subject: [PATCH 02/11] Name the layer-0 budget instead of shadowing the parameter Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/search.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/search.rs b/src/search.rs index fe62351..85f06f9 100644 --- a/src/search.rs +++ b/src/search.rs @@ -350,7 +350,7 @@ pub fn search_predicated( scratch.begin_public(graph.file.id_high_water()); // absolute cap for the layer-0 loop below: the caller's budget counted from where the // descent left off, so a wider descent does not silently shrink it - let visit_budget = stats.visits.saturating_add(visit_budget); + let layer0_budget = stats.visits.saturating_add(visit_budget); use std::collections::HashMap; let mut verdicts: HashMap = HashMap::new(); @@ -404,7 +404,7 @@ pub fn search_predicated( if results.len() >= ef && c.distance > worst { break; } - if stats.visits >= visit_budget { + if stats.visits >= layer0_budget { break; } if graph.neighbors_into(c.id, &mut nbuf).is_none() { From c3d8c82034655eb688d7b5ae066619147992ee67 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 14:08:14 -0600 Subject: [PATCH 03/11] Pin the descent's tied-distance bound, and ship the width sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the step-6 planning review. A tied-distance query was called out as unbounded: a zero vector gives every stored vector cosine distance exactly 1.0, so nothing is ever strictly worse than the beam's worst result. Measured, that is not what happens — the descent visits 608 nodes on a 50k-node graph at width 16, against an upper component of ~3300, because `search_layer` pushes a neighbour only on `d < worst` and `<` is strict. Once the result set fills, tied candidates are never pushed and the candidate heap drains after at most `ef` expansions per level. That is the real bound, and it is now a test: relaxing the comparison to `<=` would walk the whole upper component, and fails. An enforced per-level cap was tried and dropped. The same measurement that refutes the blocker also shows it cannot be sized safely — the worst ordinary query over 3000 random queries visits 788 nodes at level 1, so an `ef * UPPER_CAP` ceiling has 1.3x headroom and would clip real searches, silently degrading the recall this change exists to restore. `descent_width_sweep` is the measurement behind DESIGN.md's width table, kept runnable (ignored by default) so the numbers can be re-derived when M, ml or the prune policy changes. It reuses the corpus and insertion order the regression test already defines rather than duplicating them into an example. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/search.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++- tests/concurrent.rs | 47 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/src/search.rs b/src/search.rs index 85f06f9..e49007a 100644 --- a/src/search.rs +++ b/src/search.rs @@ -260,7 +260,7 @@ fn resolve_entry(graph: &Graph, query: &Query, stats: &mut SearchStats) -> Optio Some((id, level as u32, d)) } -/// Full search: greedy descent through upper layers, then beam at layer 0. +/// Full search: beam descent through upper layers, then the layer-0 beam at `ef`. pub fn search( graph: &Graph, query: &Query, @@ -458,6 +458,56 @@ pub fn search_predicated( (out, stats) } +#[cfg(test)] +mod descent_tests { + use super::*; + use crate::format::UPPER_CAP; + use crate::insert::{insert, InsertParams}; + use crate::PlaneFile; + + /// The descent takes no caller-supplied visit budget, so what bounds it under tied distances + /// is `search_layer`'s strict `d < worst`: once the result set is full a tied candidate is + /// never pushed, and the candidate heap drains after at most `ef` expansions. Relaxing that + /// to `<=` would make a zero query — every cosine distance exactly 1.0, and any caller can + /// send one — walk the whole upper component instead. + #[test] + fn a_tied_distance_descent_stops_at_its_visit_cap() { + let dims = 16; + let n = 6_000u32; + let path = std::env::temp_dir().join(format!("hnsw-tied-{}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 16, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + for i in 0..n { + let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); + insert(&graph, &v, ¶ms, &mut scratch).expect("insert"); + } + + let (entry_id, entry_level) = graph.file.entry_point(); + assert!(entry_level >= 1, "precondition: the graph has an upper level to descend"); + let query = Query::new(vec![0.0f32; dims]); + let entry_dist = graph.distance_to(entry_id, &query).expect("the entry point is live"); + assert_eq!(entry_dist, 1.0, "precondition: a zero query ties every stored vector at 1.0"); + + // one level only, so the bound under test is the per-level one rather than a sum + let ef = 2usize; + let mut stats = SearchStats { visits: 0 }; + beam_descend(&graph, &query, entry_id, entry_dist, 1, 0, ef, &mut scratch, &mut stats); + + // ef expansions at the maximum upper degree + let ceiling = (ef * UPPER_CAP) as u64; + assert!( + stats.visits <= ceiling, + "the tied-distance descent visited {} nodes at level 1 against a ceiling of {ceiling} — \ +level 1 holds roughly {} nodes, and a beam that pushed tied candidates would walk all of them", + stats.visits, + n / 16 + ); + let _ = std::fs::remove_file(&path); + } +} + #[cfg(test)] mod predicate_tests { use super::*; diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 09b0607..0fc0944 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -238,3 +238,50 @@ from the entry point — the descent stranded the search. First few (corpus inde let _ = std::fs::remove_file(&path); } } + +/// The measurement behind DESIGN.md's descent-width table, kept runnable so the numbers can be +/// re-derived when M, ml or the prune policy changes. Ignored by default — it reports, it does +/// not assert, and a full sweep is minutes of CPU: +/// +/// ```text +/// HNSW_SWEEP_SEEDS=700 cargo test --release --test concurrent \ +/// descent_width_sweep -- --ignored --nocapture +/// ``` +#[test] +#[ignore] +fn descent_width_sweep() { + let dims = 64; + let n = 8_000u32; + let seeds: u64 = std::env::var("HNSW_SWEEP_SEEDS").ok().and_then(|v| v.parse().ok()).unwrap_or(50); + let mut total = 0usize; + let mut bad = 0usize; + for seed in 0..seeds { + let path = std::env::temp_dir().join(format!("hnsw-sweep-{}-{seed}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 32, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + let inserted: Vec<(u32, u32)> = insertion_order(n, seed) + .into_iter() + .map(|index| { + let v = vector_for(index, dims); + (index, insert(&graph, &v, ¶ms, &mut scratch).expect("insert")) + }) + .collect(); + let misses = inserted + .iter() + .filter(|&&(index, id)| { + let (results, _) = search(&graph, &Query::new(vector_for(index, dims)), 10, 256, &mut scratch); + !results.iter().any(|&(rid, _)| rid == id) + }) + .count(); + if misses > 0 { + println!("seed {seed}: {misses} misses"); + bad += 1; + } + total += misses; + drop(graph); + let _ = std::fs::remove_file(&path); + } + println!("descent width sweep: {total} misses over {seeds} builds of {n}, {bad} builds affected"); +} From d6e17943c0d99410454d5e6609d151346dcc3ac8 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 14:35:17 -0600 Subject: [PATCH 04/11] Tighten the descent comments, and record the width and cost facts in DESIGN.md The added comments were narrating the diff rather than saying what the code cannot. Trimmed to the why in each case, with the width sweep and the cost envelope moved to DESIGN.md where a maintainer will look for them. DESIGN.md now carries three things a future change needs and the code cannot say: that read and write descent widths must match (write-only widening measures worse than baseline, 245 misses against 125, because a graph wired under one routing policy and queried under another is less navigable than one where they agree); that the trap is a property of graph shape rather than of concurrency, and does not reproduce at 32 dims or below ~8000 nodes; and that a per-level visit cap cannot be sized safely, since the obvious ef * UPPER_CAP ceiling is already exceeded by ordinary queries at 500k nodes. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- DESIGN.md | 29 +++++++++++++++++++++++++---- src/search.rs | 38 +++++++++++++++----------------------- tests/concurrent.rs | 30 +++++++++++++----------------- 3 files changed, 53 insertions(+), 44 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 44bf070..cbe06fb 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -228,10 +228,31 @@ the same graph its queries will, or nodes get their neighbors chosen from a basi never reach. The width is a compile-time constant rather than a parameter because it is a correctness floor, not a recall/latency dial — `ef` is the dial. -Two facts worth keeping when working on this. The trap is a property of graph *shape*, not of -concurrency: it reproduces single-threaded from a fixed insertion permutation, and concurrency -only shuffles that permutation. And it needs the full corpus — no seed reproduces it at 32 -dims, or at 2 000 / 4 000 nodes, so a shrunken repro is not evidence of a fix. +Three facts worth keeping when working on this. + +The trap is a property of graph *shape*, not of concurrency: it reproduces single-threaded from +a fixed insertion permutation, and concurrency only shuffles that permutation. It also needs the +full corpus — no seed reproduces it at 32 dims, or at 2 000 / 4 000 nodes, so a shrunken repro +is not evidence of a fix. `descent_width_sweep` in `tests/concurrent.rs` (ignored by default) is +the harness behind the table above. + +Read and write descent widths must match. Measured over 200 builds per cell: width 1 both sides +loses 125 nodes, read-only widening loses 37, **write-only widening loses 245 — worse than +either**, and both sides widened loses 0. `insert` seeds each level's `search_layer` from the +descent's landing point, so a graph wired under one routing policy and queried under another is +less navigable than one where they agree. This is also the upgrade story: an existing plane file +read by a new binary is the read-only row, improved but not repaired until its nodes are +re-inserted. + +Do not add a per-level visit cap to the descent without re-measuring. The obvious ceiling, +`DESCENT_EF * UPPER_CAP` = 1024, is already exceeded by ordinary queries: the worst of 3 000 +random queries visits 788 nodes at level 1 on a 50 000-node graph and 1 044 on a 500 000-node +one. A cap that binds silently degrades recall, which is the defect this exists to fix. What +bounds the pathological case instead is `search_layer`'s strict `d < worst`: with every distance +tied — a zero query ties them all at exactly 1.0 — a full result set never admits another +candidate, so the descent drains after `ef` expansions per level (measured 608 visits at 50 000 +nodes, 990 at 500 000). `a_tied_distance_descent_stops_at_its_visit_cap` fails if that `<` is +ever relaxed. **Filtering** (predicate-aware / ACORN, `filteredSearch = true` today): diff --git a/src/search.rs b/src/search.rs index e49007a..0a1d328 100644 --- a/src/search.rs +++ b/src/search.rs @@ -184,20 +184,16 @@ pub fn search_layer( out } -/// Beam width carried through every upper level of the descent. -/// -/// A width-1 descent is plain hill climbing: it halts at the first node no neighbor improves -/// on. On a clustered corpus that local minimum can sit in the wrong basin, and layer-0 -/// adjacency is intra-basin, so the layer-0 beam has no uphill edge to leave it — the query's -/// true neighborhood is then unreachable at any ef. +/// Beam width at every upper level of the descent. A width of 1 is hill climbing, which halts in +/// the first basin no neighbor improves on; layer-0 adjacency is intra-basin, so a query that +/// lands in the wrong one has no uphill edge out at any ef. See DESIGN.md §7 for the width sweep. pub const DESCENT_EF: usize = 16; -/// Beam descent through upper layers from `from_level` down to `to_level` (exclusive). -/// Each level runs a width-`ef` beam seeded by the level above's best; that level's best in -/// turn seeds the next. Returns the improved entry for the caller's layer-`to_level` search. +/// Beam descent through upper layers from `from_level` down to `to_level` (exclusive), each level +/// a width-`ef` beam seeded by the level above's best. Returns the entry for the caller's +/// layer-`to_level` search, which the caller must `begin()` a fresh epoch for. /// -/// The scratch epoch is rolled per level: a node reachable at several levels must be -/// expandable at each of them, so visited marks must not carry across the boundary. +/// A node reachable at several levels must be expandable at each, so the epoch rolls per level. pub fn beam_descend( graph: &Graph, query: &Query, @@ -214,7 +210,7 @@ pub fn beam_descend( scratch.begin(graph.file.id_high_water()); let found = search_layer(graph, query, current, current_dist, ef, level as u8, scratch, stats, None, u64::MAX); - // search_layer admits the entry itself, so `first` is never worse than the entry + // search_layer admits the entry itself, so `first` is never worse than what went in if let Some(&(id, d)) = found.first() { current = id; current_dist = d; @@ -298,8 +294,7 @@ pub fn search_filtered( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - // offset by the descent's own visits so the budget bounds layer 0, as documented, rather - // than layer 0 minus whatever the descent already spent + // absolute, so the budget bounds layer 0 rather than layer 0 less the descent let budget = if filter.is_some() { stats.visits.saturating_add((ef * filter_expansion) as u64) } else { @@ -348,8 +343,7 @@ pub fn search_predicated( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - // absolute cap for the layer-0 loop below: the caller's budget counted from where the - // descent left off, so a wider descent does not silently shrink it + // absolute, so the budget bounds layer 0 rather than layer 0 less the descent let layer0_budget = stats.visits.saturating_add(visit_budget); use std::collections::HashMap; @@ -465,11 +459,10 @@ mod descent_tests { use crate::insert::{insert, InsertParams}; use crate::PlaneFile; - /// The descent takes no caller-supplied visit budget, so what bounds it under tied distances - /// is `search_layer`'s strict `d < worst`: once the result set is full a tied candidate is - /// never pushed, and the candidate heap drains after at most `ef` expansions. Relaxing that - /// to `<=` would make a zero query — every cosine distance exactly 1.0, and any caller can - /// send one — walk the whole upper component instead. + /// The descent takes no caller-supplied visit budget. What bounds it under tied distances is + /// `search_layer`'s strict `d < worst`: a full result set admits no tied candidate, so the + /// beam drains after `ef` expansions. Relaxing that to `<=` lets a zero query — which ties + /// every cosine distance at 1.0, and any caller can send one — walk the whole upper level. #[test] fn a_tied_distance_descent_stops_at_its_visit_cap() { let dims = 16; @@ -490,8 +483,7 @@ mod descent_tests { let entry_dist = graph.distance_to(entry_id, &query).expect("the entry point is live"); assert_eq!(entry_dist, 1.0, "precondition: a zero query ties every stored vector at 1.0"); - // one level only, so the bound under test is the per-level one rather than a sum - let ef = 2usize; + let ef = 2usize; // one level, so the bound under test is per-level rather than a sum let mut stats = SearchStats { visits: 0 }; beam_descend(&graph, &query, entry_id, entry_dist, 1, 0, ef, &mut scratch, &mut stats); diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 0fc0944..3c51610 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -1,7 +1,7 @@ //! Concurrent-write torture: writers insert while readers search; then verify the graph is -//! coherent (every stored vector findable, edge lists within cap, freelist reuse works). -//! `vector_for`'s clustered corpus is also what makes the deterministic descent regression at -//! the bottom of this file bite, so the two live together rather than duplicating it. +//! coherent (every stored vector findable, edge lists within cap, freelist reuse works). The +//! deterministic descent tests at the bottom share `vector_for`: its clustered corpus is what +//! makes them bite. use hnsw_plane::distance::Query; use hnsw_plane::insert::{insert, InsertParams}; @@ -175,8 +175,8 @@ fn racing_first_inserts_all_stay_reachable() { } } -/// A reproducible insertion order over the corpus: Fisher-Yates driven by a xorshift stream, so -/// one seed names one exact graph with no thread interleaving in it. +/// Fisher-Yates over a xorshift stream: one seed names one exact graph, with no thread +/// interleaving in it. fn insertion_order(n: u32, seed: u64) -> Vec { let mut order: Vec = (0..n).collect(); let mut s = seed.wrapping_mul(0x9e37_79b9_7f4a_7c15) | 1; @@ -189,16 +189,12 @@ fn insertion_order(n: u32, seed: u64) -> Vec { order } -/// The deterministic half of `concurrent_insert_search`'s reachability assertion, and the -/// regression pin for the upper-layer descent. -/// -/// Concurrency is not what breaks the search here — it only shuffles the insertion order, which -/// this test fixes outright. A width-1 greedy descent halts at the first upper-layer node no -/// neighbor improves on; on this corpus that local minimum can sit in the wrong basin, and -/// layer-0 adjacency is intra-basin, so the layer-0 beam has no uphill edge with which to leave. -/// The query's own vector is then unreachable at any ef, which is what the sampled assertion -/// above catches only ~1.5% of the time. Both seeds trap `greedy_descend`: seed 57 loses 55 of -/// its 8000 self-queries and seed 240 loses 22. +/// `concurrent_insert_search`'s reachability assertion with the concurrency removed. Concurrency +/// only shuffles the insertion order, which this fixes outright, so what remains is the descent: +/// a width-1 hill climb halts in the first basin no neighbor improves on, and layer-0 adjacency +/// is intra-basin, leaving the query's own vector unreachable at any ef. Sampling every 97th node +/// as the test above does catches that ~1.5% of the time; these two seeds catch it every time, +/// losing 55 and 22 of their 8000 self-queries on a width-1 descent. #[test] fn a_descent_that_traps_at_a_local_minimum_still_reaches_the_true_neighborhood() { let dims = 64; @@ -240,8 +236,8 @@ from the entry point — the descent stranded the search. First few (corpus inde } /// The measurement behind DESIGN.md's descent-width table, kept runnable so the numbers can be -/// re-derived when M, ml or the prune policy changes. Ignored by default — it reports, it does -/// not assert, and a full sweep is minutes of CPU: +/// re-derived when M, ml or the prune policy changes. Ignored: it reports rather than asserts, +/// and a full sweep is minutes of CPU. /// /// ```text /// HNSW_SWEEP_SEEDS=700 cargo test --release --test concurrent \ From ca3f6e23cfafa93e236bef230c7c01fcad7e338e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 14:56:39 -0600 Subject: [PATCH 05/11] Scratch-owned beam heaps, a filter-budget test, and a width knob on the sweep Three findings from the pre-push review. search_layer allocated two BinaryHeaps and returned an owned Vec on every call. That was once per query before; the descent makes it once per upper level, on the cheap-query path the module header claims a reusable-scratch hot path for. The heaps now come from SearchScratch by the same mem::take that already served the neighbour buffer, so a query allocates the result Vec and nothing else per level. Builds benefit more than queries: insert's per-level search runs at ef_construction 200, so those were the largest heaps in the crate and they were reallocated per level per insert. The filtered and predicated budget re-basing had no test. It does now, and it fails without the fix: with a budget deliberately smaller than the descent, search_filtered returns its entry point alone instead of ten hits. descent_width_sweep was documented as the harness behind DESIGN.md's width table but had no width knob, so the table's rows could not be re-derived without editing a constant and recompiling. HNSW_SWEEP_READ_EF now drives the query-side descent independently of the compiled build-side width, which is also what separates a routing defect from a construction one: build 16 / read 1 loses 9 nodes over 25 builds where build 16 / read 16 loses none. The review also held that nothing in the suite fails if DESCENT_EF is lowered. It does: the pinned regression is red at width 8 (seed 240, 8 misses) and at width 4 (20 misses). Seed 240 is in the test precisely because it is the width-8 witness. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- DESIGN.md | 10 ++++++ src/search.rs | 79 +++++++++++++++++++++++++++++++++++++++------ tests/concurrent.rs | 42 +++++++++++++++++++++--- 3 files changed, 118 insertions(+), 13 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index cbe06fb..46a27ab 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -244,6 +244,16 @@ less navigable than one where they agree. This is also the upgrade story: an exi read by a new binary is the read-only row, improved but not repaired until its nodes are re-inserted. +`HNSW_SWEEP_READ_EF` sets the sweep's query-side width; the build side is whatever `DESCENT_EF` +is compiled as, so the four cells are two runs per value of the constant: + +```text +HNSW_SWEEP_SEEDS=200 HNSW_SWEEP_READ_EF=1 cargo test --release --test concurrent \ + descent_width_sweep -- --ignored --nocapture +HNSW_SWEEP_SEEDS=200 HNSW_SWEEP_READ_EF=16 cargo test --release --test concurrent \ + descent_width_sweep -- --ignored --nocapture +``` + Do not add a per-level visit cap to the descent without re-measuring. The obvious ceiling, `DESCENT_EF * UPPER_CAP` = 1024, is already exceeded by ordinary queries: the worst of 3 000 random queries visits 788 nodes at level 1 on a 50 000-node graph and 1 044 on a 500 000-node diff --git a/src/search.rs b/src/search.rs index 0a1d328..f7fddbf 100644 --- a/src/search.rs +++ b/src/search.rs @@ -49,11 +49,19 @@ pub struct SearchScratch { visited: Vec, epoch: u32, neighbors: Vec, + candidates: BinaryHeap, + results: BinaryHeap, } impl SearchScratch { pub fn new() -> Self { - SearchScratch { visited: Vec::new(), epoch: 0, neighbors: Vec::new() } + SearchScratch { + visited: Vec::new(), + epoch: 0, + neighbors: Vec::new(), + candidates: BinaryHeap::new(), + results: BinaryHeap::new(), + } } pub fn begin_public(&mut self, capacity: u64) { @@ -128,17 +136,20 @@ pub fn search_layer( filter: Option<&[u8]>, visit_budget: u64, ) -> Vec<(u32, f32)> { - let mut candidates = BinaryHeap::new(); - let mut results: BinaryHeap = BinaryHeap::new(); + // take() the scratch buffers to sidestep the double-borrow of scratch. The descent calls this + // once per upper level, so allocating the heaps here would be per-level, not per-query. + let mut candidates = std::mem::take(&mut scratch.candidates); + let mut results = std::mem::take(&mut scratch.results); + let mut nbuf = std::mem::take(&mut scratch.neighbors); + candidates.clear(); + results.clear(); + scratch.visit(entry); candidates.push(Candidate { distance: entry_dist, id: entry }); if bit_allowed(filter, entry) { results.push(Result_ { distance: entry_dist, id: entry }); } - // take() the scratch neighbor buffer to sidestep the double-borrow of scratch - let mut nbuf = std::mem::take(&mut scratch.neighbors); - while let Some(c) = candidates.pop() { let worst = results.peek().map(|r| r.distance).unwrap_or(f32::INFINITY); if results.len() >= ef && c.distance > worst { @@ -177,10 +188,13 @@ pub fn search_layer( } } } - scratch.neighbors = nbuf; - - let mut out: Vec<(u32, f32)> = results.into_iter().map(|r| (r.id, r.distance)).collect(); + let mut out: Vec<(u32, f32)> = results.drain().map(|r| (r.id, r.distance)).collect(); out.sort_by(|a, b| a.1.total_cmp(&b.1)); + + candidates.clear(); + scratch.neighbors = nbuf; + scratch.candidates = candidates; + scratch.results = results; out } @@ -498,6 +512,53 @@ level 1 holds roughly {} nodes, and a beam that pushed tied candidates would wal ); let _ = std::fs::remove_file(&path); } + + /// `filter_expansion` bounds layer 0, so the budget has to start counting where the descent + /// left off. Measured against a counter the descent has already advanced, a descent costing + /// more than the whole budget leaves layer 0 unable to expand even one candidate, and the + /// search returns its entry point instead of a result set — silently, since a filtered search + /// is allowed to return short. + #[test] + fn the_filter_budget_bounds_layer_zero_not_the_descent() { + let dims = 16; + let n = 6_000u32; + let path = std::env::temp_dir().join(format!("hnsw-budget-{}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 16, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + for i in 0..n { + let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); + insert(&graph, &v, ¶ms, &mut scratch).expect("insert"); + } + + let query = Query::new((0..dims).map(|d| ((97.0f32 * 0.31 + d as f32) * 0.7).sin()).collect()); + let (entry_id, entry_level) = graph.file.entry_point(); + let entry_dist = graph.distance_to(entry_id, &query).expect("the entry point is live"); + let mut descent = SearchStats { visits: 0 }; + beam_descend(&graph, &query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, &mut scratch, &mut descent); + + // a budget deliberately far below what the descent spends + let (ef, filter_expansion) = (16usize, 1usize); + assert!( + descent.visits > (ef * filter_expansion) as u64, + "precondition: the descent ({} visits) must cost more than the whole budget ({})", + descent.visits, + ef * filter_expansion + ); + let allow = vec![0xffu8; (n as usize).div_ceil(8)]; + let (hits, stats) = + search_filtered(&graph, &query, 10, ef, Some(&allow), filter_expansion, &mut scratch); + + assert_eq!(hits.len(), 10, "layer 0 got no budget of its own: {hits:?}"); + assert!( + stats.visits > descent.visits, + "layer 0 expanded nothing beyond the descent ({} total vs {} for the descent alone)", + stats.visits, + descent.visits + ); + let _ = std::fs::remove_file(&path); + } } #[cfg(test)] diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 3c51610..b07ad15 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -5,7 +5,7 @@ use hnsw_plane::distance::Query; use hnsw_plane::insert::{insert, InsertParams}; -use hnsw_plane::search::{search, SearchScratch}; +use hnsw_plane::search::{beam_descend, search, search_layer, SearchScratch, SearchStats, DESCENT_EF}; use hnsw_plane::{Graph, PlaneFile}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Barrier}; @@ -235,12 +235,40 @@ from the entry point — the descent stranded the search. First few (corpus inde } } +/// `search` at an explicit descent width — what the sweep varies, since `search` itself reads the +/// `DESCENT_EF` constant. A single-threaded build always has a live entry point, so this skips +/// `search`'s dead-entry repair and is otherwise the same sequence. +fn search_at_descent_width( + graph: &Graph, + query: &Query, + k: usize, + ef: usize, + descent_ef: usize, + scratch: &mut SearchScratch, +) -> Vec<(u32, f32)> { + let (entry_id, entry_level) = graph.file.entry_point(); + let Some(entry_dist) = graph.distance_to(entry_id, query) else { + return Vec::new(); + }; + let mut stats = SearchStats { visits: 0 }; + let (ep, ep_dist) = + beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, descent_ef, scratch, &mut stats); + scratch.begin_public(graph.file.id_high_water()); + let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX); + out.truncate(k); + out +} + /// The measurement behind DESIGN.md's descent-width table, kept runnable so the numbers can be /// re-derived when M, ml or the prune policy changes. Ignored: it reports rather than asserts, /// and a full sweep is minutes of CPU. /// +/// `HNSW_SWEEP_READ_EF` sets the query-side descent width; the build side is whatever +/// `DESCENT_EF` is compiled as. Varying them independently is what separates a routing defect +/// from a construction one — DESIGN.md §7 records that matrix. +/// /// ```text -/// HNSW_SWEEP_SEEDS=700 cargo test --release --test concurrent \ +/// HNSW_SWEEP_SEEDS=700 HNSW_SWEEP_READ_EF=8 cargo test --release --test concurrent \ /// descent_width_sweep -- --ignored --nocapture /// ``` #[test] @@ -249,6 +277,8 @@ fn descent_width_sweep() { let dims = 64; let n = 8_000u32; let seeds: u64 = std::env::var("HNSW_SWEEP_SEEDS").ok().and_then(|v| v.parse().ok()).unwrap_or(50); + let read_ef: usize = + std::env::var("HNSW_SWEEP_READ_EF").ok().and_then(|v| v.parse().ok()).unwrap_or(DESCENT_EF); let mut total = 0usize; let mut bad = 0usize; for seed in 0..seeds { @@ -267,7 +297,8 @@ fn descent_width_sweep() { let misses = inserted .iter() .filter(|&&(index, id)| { - let (results, _) = search(&graph, &Query::new(vector_for(index, dims)), 10, 256, &mut scratch); + let q = Query::new(vector_for(index, dims)); + let results = search_at_descent_width(&graph, &q, 10, 256, read_ef, &mut scratch); !results.iter().any(|&(rid, _)| rid == id) }) .count(); @@ -279,5 +310,8 @@ fn descent_width_sweep() { drop(graph); let _ = std::fs::remove_file(&path); } - println!("descent width sweep: {total} misses over {seeds} builds of {n}, {bad} builds affected"); + println!( + "descent width sweep (build {DESCENT_EF} / read {read_ef}): {total} misses over {seeds} \ +builds of {n}, {bad} builds affected" + ); } From c6f5521f67792887362b9ebd3d3a30659059426d Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 15:04:39 -0600 Subject: [PATCH 06/11] Apply the scratch heaps to search_predicated too, and floor the sweep width search_predicated hand-rolls its own layer-0 beam rather than calling search_layer, so it kept allocating its two BinaryHeaps per query while every other path had stopped. Same mem::take treatment, so predicated and plain searches now share one allocation profile. HNSW_SWEEP_READ_EF=0 would have made search_layer's `results.len() >= ef` break trip on the first candidate, so the sweep would report a clean run having expanded nothing. Floored at 1. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/search.rs | 18 +++++++++++++----- tests/concurrent.rs | 9 +++++++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/search.rs b/src/search.rs index f7fddbf..e528275 100644 --- a/src/search.rs +++ b/src/search.rs @@ -366,15 +366,19 @@ pub fn search_predicated( let mut batch: Vec = Vec::new(); let mut outstanding = 0usize; - let mut candidates = BinaryHeap::new(); - let mut results: BinaryHeap = BinaryHeap::new(); + // same scratch-owned heaps as search_layer; this path hand-rolls the layer-0 beam because + // admission is deferred on a verdict rather than decided at expansion time + let mut candidates = std::mem::take(&mut scratch.candidates); + let mut results = std::mem::take(&mut scratch.results); + let mut nbuf = std::mem::take(&mut scratch.neighbors); + candidates.clear(); + results.clear(); + scratch.visit(ep); candidates.push(Candidate { distance: ep_dist, id: ep }); speculative.push((ep, ep_dist)); batch.push(ep); - let mut nbuf = std::mem::take(&mut scratch.neighbors); - // guarded on `outstanding` rather than draining until the channel is empty: with a blocking // receive the unguarded form pays another full timeout after the last verdict lands, on every // filtered query @@ -460,9 +464,13 @@ pub fn search_predicated( false }); - let mut out: Vec<(u32, f32)> = results.into_iter().map(|r| (r.id, r.distance)).collect(); + let mut out: Vec<(u32, f32)> = results.drain().map(|r| (r.id, r.distance)).collect(); out.sort_by(|a, b| a.1.total_cmp(&b.1)); out.truncate(k); + + candidates.clear(); + scratch.candidates = candidates; + scratch.results = results; (out, stats) } diff --git a/tests/concurrent.rs b/tests/concurrent.rs index b07ad15..54141c7 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -277,8 +277,13 @@ fn descent_width_sweep() { let dims = 64; let n = 8_000u32; let seeds: u64 = std::env::var("HNSW_SWEEP_SEEDS").ok().and_then(|v| v.parse().ok()).unwrap_or(50); - let read_ef: usize = - std::env::var("HNSW_SWEEP_READ_EF").ok().and_then(|v| v.parse().ok()).unwrap_or(DESCENT_EF); + // a width of 0 makes search_layer's `results.len() >= ef` break trip immediately, so the + // sweep would report a clean run having expanded nothing + let read_ef: usize = std::env::var("HNSW_SWEEP_READ_EF") + .ok() + .and_then(|v| v.parse().ok()) + .map(|v: usize| v.max(1)) + .unwrap_or(DESCENT_EF); let mut total = 0usize; let mut bad = 0usize; for seed in 0..seeds { From 5bf4b80257d273a7b24aca7302d277aadfa29f96 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 15:11:58 -0600 Subject: [PATCH 07/11] Take the descent's last per-level allocation out, and test that it stays out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit search_layer still returned a freshly collected Vec, so the descent allocated one per upper level even after its heaps moved to the scratch. It now fills a caller-owned buffer: beam_descend reuses one across all levels, insert reuses one across its per-level searches, and the two top-level searches pass the vector they were going to return anyway. That is the last allocation that scaled with the graph's height. The claim is now checkable. tests/allocation.rs installs a counting global allocator — it needs its own test binary, since the counter is global and cargo runs tests in one binary concurrently — and asserts that per-query allocations do not grow with graph height. On a warmed scratch a search allocates once, for the vector it returns. Without the fix the test reports 3.00 allocations per query on a 2-level graph against 5.00 on a 4-level one, which is the per-level allocation made visible: every other test in the suite passes either way. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/insert.rs | 16 ++++++- src/search.rs | 30 ++++++++---- tests/allocation.rs | 110 ++++++++++++++++++++++++++++++++++++++++++++ tests/concurrent.rs | 3 +- 4 files changed, 147 insertions(+), 12 deletions(-) create mode 100644 tests/allocation.rs diff --git a/src/insert.rs b/src/insert.rs index 20b948d..d9cc28a 100644 --- a/src/insert.rs +++ b/src/insert.rs @@ -244,11 +244,23 @@ pub fn insert( // Per-level connection lists for the new node, selection-ordered. let mut connections: Vec> = vec![Vec::new(); level as usize + 1]; let mut nbuf: Vec = Vec::new(); + let mut neighbors: Vec<(u32, f32)> = Vec::with_capacity(params.ef_construction); for l in (0..=top).rev() { scratch_begin(graph, scratch); - let mut neighbors = - search_layer(graph, &query, ep, ep_dist, params.ef_construction, l, scratch, &mut stats, None, u64::MAX); + search_layer( + graph, + &query, + ep, + ep_dist, + params.ef_construction, + l, + scratch, + &mut stats, + None, + u64::MAX, + &mut neighbors, + ); neighbors.truncate(m << 1); if let Some(&(best, best_d)) = neighbors.first() { ep = best; diff --git a/src/search.rs b/src/search.rs index e528275..ca3fe77 100644 --- a/src/search.rs +++ b/src/search.rs @@ -51,6 +51,7 @@ pub struct SearchScratch { neighbors: Vec, candidates: BinaryHeap, results: BinaryHeap, + descent_out: Vec<(u32, f32)>, } impl SearchScratch { @@ -61,6 +62,7 @@ impl SearchScratch { neighbors: Vec::new(), candidates: BinaryHeap::new(), results: BinaryHeap::new(), + descent_out: Vec::new(), } } @@ -117,10 +119,14 @@ fn bit_allowed(filter: Option<&[u8]>, id: u32) -> bool { } } -/// Beam search within one layer, starting from `entry`. Level 0 reads slot adjacency; -/// upper levels read the resident upper map. Returns (id, distance) ascending by distance. +/// Beam search within one layer, starting from `entry`. Level 0 reads slot adjacency; upper +/// levels read the resident upper map. Fills `out` with (id, distance) ascending by distance. /// Assumes scratch.begin() was called for this query; entry is marked visited here. /// +/// `out` is the caller's so the descent can reuse one buffer across levels: the module's cost +/// model is one distance per visit and no allocation per query, which a fresh return `Vec` per +/// upper level would break. +/// /// `filter`: optional allow-bitset over node ids (bit i of byte i>>3). Filtered-out nodes /// are traversed (their edges route) but excluded from results — ACORN-style — with /// `visit_budget` bounding total visits so a selective filter terminates. @@ -135,7 +141,8 @@ pub fn search_layer( stats: &mut SearchStats, filter: Option<&[u8]>, visit_budget: u64, -) -> Vec<(u32, f32)> { + out: &mut Vec<(u32, f32)>, +) { // take() the scratch buffers to sidestep the double-borrow of scratch. The descent calls this // once per upper level, so allocating the heaps here would be per-level, not per-query. let mut candidates = std::mem::take(&mut scratch.candidates); @@ -188,14 +195,14 @@ pub fn search_layer( } } } - let mut out: Vec<(u32, f32)> = results.drain().map(|r| (r.id, r.distance)).collect(); + out.clear(); + out.extend(results.drain().map(|r| (r.id, r.distance))); out.sort_by(|a, b| a.1.total_cmp(&b.1)); candidates.clear(); scratch.neighbors = nbuf; scratch.candidates = candidates; scratch.results = results; - out } /// Beam width at every upper level of the descent. A width of 1 is hill climbing, which halts in @@ -219,11 +226,13 @@ pub fn beam_descend( scratch: &mut SearchScratch, stats: &mut SearchStats, ) -> (u32, f32) { + let mut found = std::mem::take(&mut scratch.descent_out); let mut level = from_level.min(MAX_UPPER_LEVELS as u32); while level > to_level { scratch.begin(graph.file.id_high_water()); - let found = - search_layer(graph, query, current, current_dist, ef, level as u8, scratch, stats, None, u64::MAX); + search_layer( + graph, query, current, current_dist, ef, level as u8, scratch, stats, None, u64::MAX, &mut found, + ); // search_layer admits the entry itself, so `first` is never worse than what went in if let Some(&(id, d)) = found.first() { current = id; @@ -231,6 +240,7 @@ pub fn beam_descend( } level -= 1; } + scratch.descent_out = found; (current, current_dist) } @@ -285,7 +295,8 @@ pub fn search( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin(graph.file.id_high_water()); - let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX); + let mut out = Vec::with_capacity(ef); + search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); (out, stats) } @@ -314,7 +325,8 @@ pub fn search_filtered( } else { u64::MAX }; - let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, filter, budget); + let mut out = Vec::with_capacity(ef); + search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, filter, budget, &mut out); out.truncate(k); (out, stats) } diff --git a/tests/allocation.rs b/tests/allocation.rs new file mode 100644 index 0000000..7fa9efd --- /dev/null +++ b/tests/allocation.rs @@ -0,0 +1,110 @@ +//! The module header calls the search path zero-copy with a reusable scratch. This binary is the +//! only place that can hold a counting global allocator, so it owns the one test that checks it: +//! per-query allocations must not scale with the graph's height. A descent that allocated per +//! upper level would pass every other test in the suite. + +use hnsw_plane::distance::Query; +use hnsw_plane::insert::{insert, InsertParams}; +use hnsw_plane::search::{search, SearchScratch}; +use hnsw_plane::{Graph, PlaneFile}; +use std::alloc::{GlobalAlloc, Layout, System}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +static ALLOCATIONS: AtomicUsize = AtomicUsize::new(0); +static COUNTING: AtomicUsize = AtomicUsize::new(0); + +struct CountingAllocator; + +unsafe impl GlobalAlloc for CountingAllocator { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + if COUNTING.load(Ordering::Relaxed) != 0 { + ALLOCATIONS.fetch_add(1, Ordering::Relaxed); + } + System.alloc(layout) + } + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + System.dealloc(ptr, layout) + } + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + if COUNTING.load(Ordering::Relaxed) != 0 { + ALLOCATIONS.fetch_add(1, Ordering::Relaxed); + } + System.realloc(ptr, layout, new_size) + } +} + +#[global_allocator] +static ALLOCATOR: CountingAllocator = CountingAllocator; + +/// Allocations attributable to one `search` on a warmed scratch, averaged over `queries`. +fn allocations_per_search(graph: &Graph, dims: usize, scratch: &mut SearchScratch, queries: usize) -> f64 { + let vector = |i: usize| -> Vec { + (0..dims).map(|d| ((i as f32 * 0.11 + d as f32) * 0.9).sin()).collect() + }; + // warm every reusable buffer to its steady-state capacity first + for i in 0..8 { + let _ = search(graph, &Query::new(vector(i)), 10, 64, scratch); + } + let queries_prepared: Vec = (0..queries).map(|i| Query::new(vector(i + 100))).collect(); + + ALLOCATIONS.store(0, Ordering::Relaxed); + COUNTING.store(1, Ordering::Relaxed); + for query in &queries_prepared { + let (hits, _) = search(graph, query, 10, 64, scratch); + std::hint::black_box(hits); + } + COUNTING.store(0, Ordering::Relaxed); + ALLOCATIONS.load(Ordering::Relaxed) as f64 / queries as f64 +} + +fn build(path: &std::path::Path, n: u32, dims: usize) -> Graph { + let _ = std::fs::remove_file(path); + let graph = Graph::new(PlaneFile::create(path, dims, 16, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + for i in 0..n { + let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); + insert(&graph, &v, ¶ms, &mut scratch).expect("insert"); + } + graph +} + +/// A taller graph means more upper levels to descend. If the descent allocated per level, the +/// per-query count would rise with height; it must not. +#[test] +fn a_search_does_not_allocate_per_upper_level() { + let dims = 16; + let pid = std::process::id(); + let shallow_path = std::env::temp_dir().join(format!("hnsw-alloc-shallow-{pid}.hnsw")); + let tall_path = std::env::temp_dir().join(format!("hnsw-alloc-tall-{pid}.hnsw")); + + let shallow = build(&shallow_path, 2_000, dims); + let tall = build(&tall_path, 60_000, dims); + let shallow_levels = shallow.file.entry_point().1; + let tall_levels = tall.file.entry_point().1; + assert!( + tall_levels >= shallow_levels + 2, + "precondition: the two graphs must differ in height ({shallow_levels} vs {tall_levels})" + ); + + let mut scratch = SearchScratch::new(); + let shallow_allocs = allocations_per_search(&shallow, dims, &mut scratch, 200); + let tall_allocs = allocations_per_search(&tall, dims, &mut scratch, 200); + + // the returned Vec, and nothing that scales with the descent + assert!( + tall_allocs <= shallow_allocs + 0.5, + "search allocates per upper level: {shallow_allocs:.2}/query at {shallow_levels} levels \ +vs {tall_allocs:.2}/query at {tall_levels}" + ); + assert!( + tall_allocs <= 2.0, + "search allocates {tall_allocs:.2} times per query on a warmed scratch; the result vector \ +should be the only one" + ); + + drop(shallow); + drop(tall); + let _ = std::fs::remove_file(&shallow_path); + let _ = std::fs::remove_file(&tall_path); +} diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 54141c7..2bf3193 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -254,7 +254,8 @@ fn search_at_descent_width( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, descent_ef, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - let mut out = search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX); + let mut out = Vec::with_capacity(ef); + search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); out } From a080525882c1e1eaa6e0ff2e3b3d18bd789e0f0a Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 15:20:56 -0600 Subject: [PATCH 08/11] Test the predicate budget rebase, and trim comments that read back the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit search_predicated carries its own layer-0 loop, so the filtered budget test did not cover it — and all three existing predicate tests pass 64 * 24 = 1536, orders above what a descent costs, so they hold with or without the rebase. The new test passes a budget of 64 against a descent that costs more than that, and fails without the fix by returning the descent's landing point alone. Comments: dropped the duplicated budget note, the one reading `ef * UPPER_CAP` back as prose, and the sentences arguing the change to a reviewer rather than telling the next reader something. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/search.rs | 68 +++++++++++++++++++++++++++++++++++++++------ tests/concurrent.rs | 3 +- 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/src/search.rs b/src/search.rs index ca3fe77..14e01c8 100644 --- a/src/search.rs +++ b/src/search.rs @@ -123,9 +123,7 @@ fn bit_allowed(filter: Option<&[u8]>, id: u32) -> bool { /// levels read the resident upper map. Fills `out` with (id, distance) ascending by distance. /// Assumes scratch.begin() was called for this query; entry is marked visited here. /// -/// `out` is the caller's so the descent can reuse one buffer across levels: the module's cost -/// model is one distance per visit and no allocation per query, which a fresh return `Vec` per -/// upper level would break. +/// `out` is the caller's so the descent can reuse one buffer across all levels. /// /// `filter`: optional allow-bitset over node ids (bit i of byte i>>3). Filtered-out nodes /// are traversed (their edges route) but excluded from results — ACORN-style — with @@ -143,8 +141,7 @@ pub fn search_layer( visit_budget: u64, out: &mut Vec<(u32, f32)>, ) { - // take() the scratch buffers to sidestep the double-borrow of scratch. The descent calls this - // once per upper level, so allocating the heaps here would be per-level, not per-query. + // take() the scratch buffers to sidestep the double-borrow of scratch let mut candidates = std::mem::take(&mut scratch.candidates); let mut results = std::mem::take(&mut scratch.results); let mut nbuf = std::mem::take(&mut scratch.neighbors); @@ -369,7 +366,6 @@ pub fn search_predicated( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - // absolute, so the budget bounds layer 0 rather than layer 0 less the descent let layer0_budget = stats.visits.saturating_add(visit_budget); use std::collections::HashMap; @@ -378,8 +374,8 @@ pub fn search_predicated( let mut batch: Vec = Vec::new(); let mut outstanding = 0usize; - // same scratch-owned heaps as search_layer; this path hand-rolls the layer-0 beam because - // admission is deferred on a verdict rather than decided at expansion time + // this path hand-rolls the layer-0 beam because admission waits on a verdict rather than + // being decided at expansion time; the scratch heaps are search_layer's let mut candidates = std::mem::take(&mut scratch.candidates); let mut results = std::mem::take(&mut scratch.results); let mut nbuf = std::mem::take(&mut scratch.neighbors); @@ -521,7 +517,6 @@ mod descent_tests { let mut stats = SearchStats { visits: 0 }; beam_descend(&graph, &query, entry_id, entry_dist, 1, 0, ef, &mut scratch, &mut stats); - // ef expansions at the maximum upper degree let ceiling = (ef * UPPER_CAP) as u64; assert!( stats.visits <= ceiling, @@ -579,6 +574,61 @@ level 1 holds roughly {} nodes, and a beam that pushed tied candidates would wal ); let _ = std::fs::remove_file(&path); } + + /// The same contract on the predicate path, which carries its own layer-0 loop: a host budget + /// smaller than the descent must still buy layer-0 visits. `predicate_tests` all pass + /// `64 * 24`, orders above what a descent costs, so they hold either way. + #[test] + fn the_predicate_visit_budget_bounds_layer_zero_not_the_descent() { + let dims = 16; + let n = 6_000u32; + let path = std::env::temp_dir().join(format!("hnsw-predbudget-{}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 16, n as u64 + 1024).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + for i in 0..n { + let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); + insert(&graph, &v, ¶ms, &mut scratch).expect("insert"); + } + + let query = Query::new((0..dims).map(|d| ((97.0f32 * 0.31 + d as f32) * 0.7).sin()).collect()); + let (entry_id, entry_level) = graph.file.entry_point(); + let entry_dist = graph.distance_to(entry_id, &query).expect("the entry point is live"); + let mut descent = SearchStats { visits: 0 }; + beam_descend(&graph, &query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, &mut scratch, &mut descent); + + let budget = 64u64; + assert!( + descent.visits > budget, + "precondition: the descent ({} visits) must cost more than the whole budget ({budget})", + descent.visits + ); + + let (req_tx, req_rx) = std::sync::mpsc::channel::>(); + let (res_tx, res_rx) = std::sync::mpsc::channel::<(Vec, Vec)>(); + let worker = std::thread::spawn(move || { + while let Ok(ids) = req_rx.recv() { + let verdicts = vec![1u8; ids.len()]; + if res_tx.send((ids, verdicts)).is_err() { + break; + } + } + }); + let mut pipe = PredicatePipe { dispatch: Box::new(move |ids| req_tx.send(ids).is_ok()), rx: res_rx }; + let (hits, stats) = search_predicated(&graph, &query, 10, 16, &mut pipe, budget, &mut scratch); + + assert_eq!(hits.len(), 10, "layer 0 got no budget of its own: {hits:?}"); + assert!( + stats.visits > descent.visits, + "layer 0 expanded nothing beyond the descent ({} total vs {} for the descent alone)", + stats.visits, + descent.visits + ); + drop(pipe); + worker.join().unwrap(); + let _ = std::fs::remove_file(&path); + } } #[cfg(test)] diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 2bf3193..5a44625 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -278,8 +278,7 @@ fn descent_width_sweep() { let dims = 64; let n = 8_000u32; let seeds: u64 = std::env::var("HNSW_SWEEP_SEEDS").ok().and_then(|v| v.parse().ok()).unwrap_or(50); - // a width of 0 makes search_layer's `results.len() >= ef` break trip immediately, so the - // sweep would report a clean run having expanded nothing + // 0 would silently mean width 1, since the entry is admitted before any cap check let read_ef: usize = std::env::var("HNSW_SWEEP_READ_EF") .ok() .and_then(|v| v.parse().ok()) From c6acc3c3cc285f7f6126f1ed5ac2ad83e21e77cd Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 16:39:01 -0600 Subject: [PATCH 09/11] Bound the result reservation by the plane, not by the caller's ef MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filling a caller-owned buffer meant reserving it up front, and `ef` reaches that reservation from an unvalidated u32 at the NAPI boundary — `ef as usize` with no ceiling. The previous `collect()` sized to the results actually found, so this was newly introduced: a query carrying `ef: 4294967295`, a config typo or an API that exposes efSearch to its caller, asks for 34 GB, and handle_alloc_error answers by aborting the process. Uncatchable from JS, and it takes every in-flight query with it. Linux overcommit usually hides it; the Windows CI runner and strict-overcommit hosts do not. A result set cannot exceed the ids the plane has ever allocated, so the reservation is now `ef.min(id_high_water())` — still one reservation, since a growing push loop would reallocate about seven times at ef 64 and break the allocation test's own bound. The allocation test builds at ef_construction 24 rather than the default 200: it measures search allocations, so graph quality is irrelevant and there is no reason to put a full-quality 60k build on three CI runners. Its height precondition now says in the failure message that it derives from `ml`, so tuning that reads as what it is rather than as an allocation regression. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/insert.rs | 3 ++- src/search.rs | 44 ++++++++++++++++++++++++++++++++++++++++---- tests/allocation.rs | 9 ++++++--- tests/concurrent.rs | 2 +- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/src/insert.rs b/src/insert.rs index d9cc28a..4fafca8 100644 --- a/src/insert.rs +++ b/src/insert.rs @@ -244,7 +244,8 @@ pub fn insert( // Per-level connection lists for the new node, selection-ordered. let mut connections: Vec> = vec![Vec::new(); level as usize + 1]; let mut nbuf: Vec = Vec::new(); - let mut neighbors: Vec<(u32, f32)> = Vec::with_capacity(params.ef_construction); + let mut neighbors: Vec<(u32, f32)> = + Vec::with_capacity(params.ef_construction.min(graph.file.id_high_water() as usize)); for l in (0..=top).rev() { scratch_begin(graph, scratch); diff --git a/src/search.rs b/src/search.rs index 14e01c8..b9e81ed 100644 --- a/src/search.rs +++ b/src/search.rs @@ -44,6 +44,16 @@ impl PartialOrd for Result_ { } } +/// Capacity for a layer's result buffer. `ef` reaches this from an unvalidated `u32` at the NAPI +/// boundary, and a result set cannot exceed the ids the plane has ever allocated, so reserving +/// `ef` outright turns a caller's typo into a multi-gigabyte request and an allocator abort. One +/// reservation, not a growing push loop: the descent's no-allocation-per-level property is what +/// `tests/allocation.rs` pins. +#[inline] +fn result_capacity(graph: &Graph, ef: usize) -> usize { + ef.min(graph.file.id_high_water() as usize) +} + /// Reusable per-thread search scratch. pub struct SearchScratch { visited: Vec, @@ -292,7 +302,7 @@ pub fn search( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin(graph.file.id_high_water()); - let mut out = Vec::with_capacity(ef); + let mut out = Vec::with_capacity(result_capacity(graph, ef)); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); (out, stats) @@ -316,13 +326,12 @@ pub fn search_filtered( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - // absolute, so the budget bounds layer 0 rather than layer 0 less the descent let budget = if filter.is_some() { stats.visits.saturating_add((ef * filter_expansion) as u64) } else { u64::MAX }; - let mut out = Vec::with_capacity(ef); + let mut out = Vec::with_capacity(result_capacity(graph, ef)); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, filter, budget, &mut out); out.truncate(k); (out, stats) @@ -375,7 +384,7 @@ pub fn search_predicated( let mut outstanding = 0usize; // this path hand-rolls the layer-0 beam because admission waits on a verdict rather than - // being decided at expansion time; the scratch heaps are search_layer's + // being decided at expansion time let mut candidates = std::mem::take(&mut scratch.candidates); let mut results = std::mem::take(&mut scratch.results); let mut nbuf = std::mem::take(&mut scratch.neighbors); @@ -575,6 +584,33 @@ level 1 holds roughly {} nodes, and a beam that pushed tied candidates would wal let _ = std::fs::remove_file(&path); } + /// `ef` arrives from an unvalidated `u32` at the NAPI boundary. Reserving it outright turns a + /// caller's typo into a request for tens of gigabytes, which `handle_alloc_error` answers by + /// aborting the process — uncatchable from JS, and it takes every in-flight query with it. + #[test] + fn an_absurd_ef_does_not_reserve_by_it() { + let dims = 8; + let n = 64u32; + let path = std::env::temp_dir().join(format!("hnsw-absurdef-{}.hnsw", std::process::id())); + let _ = std::fs::remove_file(&path); + let graph = Graph::new(PlaneFile::create(&path, dims, 8, n as u64 + 16).expect("create")); + let params = InsertParams::default(); + let mut scratch = SearchScratch::new(); + for i in 0..n { + let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); + insert(&graph, &v, ¶ms, &mut scratch).expect("insert"); + } + + let query = Query::new((0..dims).map(|d| ((7.0f32 * 0.31 + d as f32) * 0.7).sin()).collect()); + let (hits, _) = search(&graph, &query, 5, u32::MAX as usize, &mut scratch); + assert_eq!(hits.len(), 5, "an absurd ef must still answer from a {n}-node plane"); + assert!( + result_capacity(&graph, u32::MAX as usize) <= n as usize + 16, + "the reservation is bounded by the plane, not by ef" + ); + let _ = std::fs::remove_file(&path); + } + /// The same contract on the predicate path, which carries its own layer-0 loop: a host budget /// smaller than the descent must still buy layer-0 visits. `predicate_tests` all pass /// `64 * 24`, orders above what a descent costs, so they hold either way. diff --git a/tests/allocation.rs b/tests/allocation.rs index 7fa9efd..02bf6a6 100644 --- a/tests/allocation.rs +++ b/tests/allocation.rs @@ -60,7 +60,9 @@ fn allocations_per_search(graph: &Graph, dims: usize, scratch: &mut SearchScratc fn build(path: &std::path::Path, n: u32, dims: usize) -> Graph { let _ = std::fs::remove_file(path); let graph = Graph::new(PlaneFile::create(path, dims, 16, n as u64 + 1024).expect("create")); - let params = InsertParams::default(); + // graph quality is irrelevant here — only its height is — so build at a fraction of the + // default ef_construction rather than adding a full-quality 60k build to every CI run + let params = InsertParams { ef_construction: 24, ..InsertParams::default() }; let mut scratch = SearchScratch::new(); for i in 0..n { let v: Vec = (0..dims).map(|d| ((i as f32 * 0.31 + d as f32) * 0.7).sin()).collect(); @@ -84,14 +86,15 @@ fn a_search_does_not_allocate_per_upper_level() { let tall_levels = tall.file.entry_point().1; assert!( tall_levels >= shallow_levels + 2, - "precondition: the two graphs must differ in height ({shallow_levels} vs {tall_levels})" + "precondition: the two graphs must differ in height ({shallow_levels} vs {tall_levels}). \ +This is derived from `ml` and `level_for`; if either was tuned, re-pick the two node counts \ +rather than reading this as an allocation regression" ); let mut scratch = SearchScratch::new(); let shallow_allocs = allocations_per_search(&shallow, dims, &mut scratch, 200); let tall_allocs = allocations_per_search(&tall, dims, &mut scratch, 200); - // the returned Vec, and nothing that scales with the descent assert!( tall_allocs <= shallow_allocs + 0.5, "search allocates per upper level: {shallow_allocs:.2}/query at {shallow_levels} levels \ diff --git a/tests/concurrent.rs b/tests/concurrent.rs index 5a44625..f59da35 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -254,7 +254,7 @@ fn search_at_descent_width( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, descent_ef, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - let mut out = Vec::with_capacity(ef); + let mut out = Vec::with_capacity(ef.min(graph.file.id_high_water() as usize)); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); out From 611ae4798f5f55e55829f838c2962e3b4f010887 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 17:12:53 -0600 Subject: [PATCH 10/11] Stop sizing any allocation by ef, and scope the allocation counter to one thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit bounded the result reservation by the plane's high-water. It turns out no reservation was needed: `out` is filled by one `extend` off an exact-size drain, which reserves once for the results actually found. So `Vec::new()` allocates exactly the same number of times — the allocation test still passes its `<= 2 per query` bound — and nothing is sized by `ef` at all. That removes the last path from an unvalidated u32 to an allocation size, rather than making it smaller: on a fifty-million-node plane the bounded form still permitted a 400 MB request. tests/allocation.rs installs a process-global allocator, so a plain counting flag folded in every other thread in the binary, the test harness's own included. The counter is now keyed to the measuring thread's `pthread_self`, which is used rather than `thread::current().id()` because the latter can allocate and allocating inside the allocator recurses. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- src/insert.rs | 3 +-- src/search.rs | 30 +++++++++--------------------- tests/allocation.rs | 28 ++++++++++++++++++++++------ tests/concurrent.rs | 2 +- 4 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/insert.rs b/src/insert.rs index 4fafca8..6e64a2f 100644 --- a/src/insert.rs +++ b/src/insert.rs @@ -244,8 +244,7 @@ pub fn insert( // Per-level connection lists for the new node, selection-ordered. let mut connections: Vec> = vec![Vec::new(); level as usize + 1]; let mut nbuf: Vec = Vec::new(); - let mut neighbors: Vec<(u32, f32)> = - Vec::with_capacity(params.ef_construction.min(graph.file.id_high_water() as usize)); + let mut neighbors: Vec<(u32, f32)> = Vec::new(); for l in (0..=top).rev() { scratch_begin(graph, scratch); diff --git a/src/search.rs b/src/search.rs index b9e81ed..adc5ab9 100644 --- a/src/search.rs +++ b/src/search.rs @@ -44,16 +44,6 @@ impl PartialOrd for Result_ { } } -/// Capacity for a layer's result buffer. `ef` reaches this from an unvalidated `u32` at the NAPI -/// boundary, and a result set cannot exceed the ids the plane has ever allocated, so reserving -/// `ef` outright turns a caller's typo into a multi-gigabyte request and an allocator abort. One -/// reservation, not a growing push loop: the descent's no-allocation-per-level property is what -/// `tests/allocation.rs` pins. -#[inline] -fn result_capacity(graph: &Graph, ef: usize) -> usize { - ef.min(graph.file.id_high_water() as usize) -} - /// Reusable per-thread search scratch. pub struct SearchScratch { visited: Vec, @@ -133,7 +123,9 @@ fn bit_allowed(filter: Option<&[u8]>, id: u32) -> bool { /// levels read the resident upper map. Fills `out` with (id, distance) ascending by distance. /// Assumes scratch.begin() was called for this query; entry is marked visited here. /// -/// `out` is the caller's so the descent can reuse one buffer across all levels. +/// `out` is the caller's so the descent can reuse one buffer across all levels. It is filled by +/// one `extend` off an exact-size drain, so it takes at most one allocation sized to the results +/// actually found — never to `ef`, which arrives unvalidated from the NAPI boundary. /// /// `filter`: optional allow-bitset over node ids (bit i of byte i>>3). Filtered-out nodes /// are traversed (their edges route) but excluded from results — ACORN-style — with @@ -302,7 +294,7 @@ pub fn search( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, DESCENT_EF, scratch, &mut stats); scratch.begin(graph.file.id_high_water()); - let mut out = Vec::with_capacity(result_capacity(graph, ef)); + let mut out = Vec::new(); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); (out, stats) @@ -331,7 +323,7 @@ pub fn search_filtered( } else { u64::MAX }; - let mut out = Vec::with_capacity(result_capacity(graph, ef)); + let mut out = Vec::new(); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, filter, budget, &mut out); out.truncate(k); (out, stats) @@ -584,11 +576,11 @@ level 1 holds roughly {} nodes, and a beam that pushed tied candidates would wal let _ = std::fs::remove_file(&path); } - /// `ef` arrives from an unvalidated `u32` at the NAPI boundary. Reserving it outright turns a - /// caller's typo into a request for tens of gigabytes, which `handle_alloc_error` answers by - /// aborting the process — uncatchable from JS, and it takes every in-flight query with it. + /// `ef` arrives from an unvalidated `u32` at the NAPI boundary. Sizing any allocation by it + /// turns a caller's typo into a request for tens of gigabytes, which `handle_alloc_error` + /// answers by aborting — uncatchable from JS, and it takes every in-flight query with it. #[test] - fn an_absurd_ef_does_not_reserve_by_it() { + fn an_absurd_ef_answers_instead_of_reserving_by_it() { let dims = 8; let n = 64u32; let path = std::env::temp_dir().join(format!("hnsw-absurdef-{}.hnsw", std::process::id())); @@ -604,10 +596,6 @@ level 1 holds roughly {} nodes, and a beam that pushed tied candidates would wal let query = Query::new((0..dims).map(|d| ((7.0f32 * 0.31 + d as f32) * 0.7).sin()).collect()); let (hits, _) = search(&graph, &query, 5, u32::MAX as usize, &mut scratch); assert_eq!(hits.len(), 5, "an absurd ef must still answer from a {n}-node plane"); - assert!( - result_capacity(&graph, u32::MAX as usize) <= n as usize + 16, - "the reservation is bounded by the plane, not by ef" - ); let _ = std::fs::remove_file(&path); } diff --git a/tests/allocation.rs b/tests/allocation.rs index 02bf6a6..63f5465 100644 --- a/tests/allocation.rs +++ b/tests/allocation.rs @@ -8,16 +8,32 @@ use hnsw_plane::insert::{insert, InsertParams}; use hnsw_plane::search::{search, SearchScratch}; use hnsw_plane::{Graph, PlaneFile}; use std::alloc::{GlobalAlloc, Layout, System}; -use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; static ALLOCATIONS: AtomicUsize = AtomicUsize::new(0); -static COUNTING: AtomicUsize = AtomicUsize::new(0); +/// The thread whose allocations count, as a raw `pthread_t`. A global allocator sees every +/// thread in the binary — the test harness's own included — so counting unconditionally would +/// fold their allocations into the measurement. `pthread_self` is used rather than +/// `thread::current().id()` because the latter can allocate, and allocating inside the allocator +/// recurses. Zero means counting is off. +static COUNTING_THREAD: AtomicU64 = AtomicU64::new(0); + +#[inline] +fn this_thread() -> u64 { + unsafe { libc::pthread_self() as u64 } +} + +#[inline] +fn counting_here() -> bool { + let t = COUNTING_THREAD.load(Ordering::Relaxed); + t != 0 && t == this_thread() +} struct CountingAllocator; unsafe impl GlobalAlloc for CountingAllocator { unsafe fn alloc(&self, layout: Layout) -> *mut u8 { - if COUNTING.load(Ordering::Relaxed) != 0 { + if counting_here() { ALLOCATIONS.fetch_add(1, Ordering::Relaxed); } System.alloc(layout) @@ -26,7 +42,7 @@ unsafe impl GlobalAlloc for CountingAllocator { System.dealloc(ptr, layout) } unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { - if COUNTING.load(Ordering::Relaxed) != 0 { + if counting_here() { ALLOCATIONS.fetch_add(1, Ordering::Relaxed); } System.realloc(ptr, layout, new_size) @@ -48,12 +64,12 @@ fn allocations_per_search(graph: &Graph, dims: usize, scratch: &mut SearchScratc let queries_prepared: Vec = (0..queries).map(|i| Query::new(vector(i + 100))).collect(); ALLOCATIONS.store(0, Ordering::Relaxed); - COUNTING.store(1, Ordering::Relaxed); + COUNTING_THREAD.store(this_thread(), Ordering::Relaxed); for query in &queries_prepared { let (hits, _) = search(graph, query, 10, 64, scratch); std::hint::black_box(hits); } - COUNTING.store(0, Ordering::Relaxed); + COUNTING_THREAD.store(0, Ordering::Relaxed); ALLOCATIONS.load(Ordering::Relaxed) as f64 / queries as f64 } diff --git a/tests/concurrent.rs b/tests/concurrent.rs index f59da35..5040859 100644 --- a/tests/concurrent.rs +++ b/tests/concurrent.rs @@ -254,7 +254,7 @@ fn search_at_descent_width( let (ep, ep_dist) = beam_descend(graph, query, entry_id, entry_dist, entry_level, 0, descent_ef, scratch, &mut stats); scratch.begin_public(graph.file.id_high_water()); - let mut out = Vec::with_capacity(ef.min(graph.file.id_high_water() as usize)); + let mut out = Vec::new(); search_layer(graph, query, ep, ep_dist, ef, 0, scratch, &mut stats, None, u64::MAX, &mut out); out.truncate(k); out From 8a5eb71cfe9a46fcc0d5417222c176c57cf4da89 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 3 Sep 2026 17:21:55 -0600 Subject: [PATCH 11/11] =?UTF-8?q?Make=20the=20allocation=20counter=20porta?= =?UTF-8?q?ble=20=E2=80=94=20pthread=5Fself=20is=20not=20on=20Windows?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI builds on windows-latest, where `libc::pthread_self` does not exist, so the previous commit would have broken the build there. Both review legs caught it independently. The counting flag is now a `thread_local!` `Cell` with `const` init: no libc, no platform gate, and reading it neither lazily initializes nor registers a destructor, either of which would allocate — and allocating inside the allocator recurses. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01AizJVayzmA8WFhdxPz1FhV --- tests/allocation.rs | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/tests/allocation.rs b/tests/allocation.rs index 63f5465..41210bb 100644 --- a/tests/allocation.rs +++ b/tests/allocation.rs @@ -8,25 +8,23 @@ use hnsw_plane::insert::{insert, InsertParams}; use hnsw_plane::search::{search, SearchScratch}; use hnsw_plane::{Graph, PlaneFile}; use std::alloc::{GlobalAlloc, Layout, System}; -use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering}; +use std::cell::Cell; +use std::sync::atomic::{AtomicUsize, Ordering}; static ALLOCATIONS: AtomicUsize = AtomicUsize::new(0); -/// The thread whose allocations count, as a raw `pthread_t`. A global allocator sees every -/// thread in the binary — the test harness's own included — so counting unconditionally would -/// fold their allocations into the measurement. `pthread_self` is used rather than -/// `thread::current().id()` because the latter can allocate, and allocating inside the allocator -/// recurses. Zero means counting is off. -static COUNTING_THREAD: AtomicU64 = AtomicU64::new(0); -#[inline] -fn this_thread() -> u64 { - unsafe { libc::pthread_self() as u64 } +thread_local! { + /// A global allocator sees every thread in the binary, the test harness's own included, so a + /// process-wide flag would fold their allocations into the measurement. Scoping it per thread + /// keeps the count to the one doing the searching. `const` init on a `Cell` so that reading it + /// neither lazily initializes nor registers a destructor — either would allocate, and + /// allocating inside the allocator recurses. + static COUNTING: Cell = const { Cell::new(false) }; } #[inline] fn counting_here() -> bool { - let t = COUNTING_THREAD.load(Ordering::Relaxed); - t != 0 && t == this_thread() + COUNTING.try_with(|c| c.get()).unwrap_or(false) } struct CountingAllocator; @@ -57,27 +55,25 @@ fn allocations_per_search(graph: &Graph, dims: usize, scratch: &mut SearchScratc let vector = |i: usize| -> Vec { (0..dims).map(|d| ((i as f32 * 0.11 + d as f32) * 0.9).sin()).collect() }; - // warm every reusable buffer to its steady-state capacity first for i in 0..8 { let _ = search(graph, &Query::new(vector(i)), 10, 64, scratch); } let queries_prepared: Vec = (0..queries).map(|i| Query::new(vector(i + 100))).collect(); ALLOCATIONS.store(0, Ordering::Relaxed); - COUNTING_THREAD.store(this_thread(), Ordering::Relaxed); + COUNTING.with(|c| c.set(true)); for query in &queries_prepared { let (hits, _) = search(graph, query, 10, 64, scratch); std::hint::black_box(hits); } - COUNTING_THREAD.store(0, Ordering::Relaxed); + COUNTING.with(|c| c.set(false)); ALLOCATIONS.load(Ordering::Relaxed) as f64 / queries as f64 } fn build(path: &std::path::Path, n: u32, dims: usize) -> Graph { let _ = std::fs::remove_file(path); let graph = Graph::new(PlaneFile::create(path, dims, 16, n as u64 + 1024).expect("create")); - // graph quality is irrelevant here — only its height is — so build at a fraction of the - // default ef_construction rather than adding a full-quality 60k build to every CI run + // only the graph's height matters here, not its quality let params = InsertParams { ef_construction: 24, ..InsertParams::default() }; let mut scratch = SearchScratch::new(); for i in 0..n {