Free the old HNSW index before reload and cap ORT thread pools - #18
Conversation
Two fixes from the #736 (travsr repo) memory RCA: - VecIndex::knn reload: free the old HNSW graph before loading the updated file. The old *self = try_load(..) built the replacement while the previous index was still alive - a transient 2x of the full index size at exactly the moment a just-finished reindex had already elevated memory. - OrtBackend: cap intra-op threads at available_parallelism() (cgroup quota-aware on Linux) and inter-op at 1. Left unset, ONNX Runtime sizes its pool from the HOST physical core count via its own topology probe, so a container limited to 2 CPUs on a 64-core host got ~64 spinning threads and permanent throttling.
Completes items 4 and 7 (embed side) of the #736 memory RCA: - VecIndex::try_serve: the daemon serving path now memory-maps the index (usearch view) on Linux/macOS instead of copying it into RAM, so the OS pages it in on demand and can evict under pressure. A viewed index is immutable, so the lazy-embed add becomes a no-op (the vector still persists to embed.db and enters the index on the next reindex) and reloads re-view. Windows keeps the load fallback: a live file mapping there takes a sharing lock that would break the reindex sidecars build_from_db save. - Both reindex paths stop materialising the whole pending corpus. A COUNT query provides totals, then PENDING_CHUNK_ROWS (50k) rows are fetched, embedded, and committed per pass; committed chunks drop out of the NOT EXISTS filter so the loop needs no OFFSET and always terminates. The parallel path keeps its shared-batch-queue worker design per chunk, with the WS3 cancel watcher created per chunk so it captures that chunks n_batches.
451fa18 to
f8d28ae
Compare
The chunked pending queries returned collect() as the block tail expression, whose temporaries outlive block locals: the MappedRows borrow of stmt lived past stmt itself (E0597, caught by CI on every target). Binding to a local first ends the borrow before stmt drops, the same shape the pre-chunking code used.
On Windows try_serve falls back to load(), where usearch refuses an insert without a prior reserve() - the production lazy-embed path ignores exactly that error, but the test unwrapped it and panicked on the MSVC CI job. The contract under test is that a lazy add on a serving handle is non-fatal (and a no-op on a viewed index), not that it succeeds.
raj-rkv
left a comment
raj-rkv
left a comment
There was a problem hiding this comment.
The two changes the description covers are both correct, and I verified them rather than reading the comments:
- The reload genuinely frees before loading,
last_modifiedis left unadvanced on a failed load so the nextknn()retries, and theviewedbranch keeps a serving handle from silently becoming a loading one. try_serveandtry_loadare cleanly separated at every call site: serving (NomicPlugin::load, both late-load paths) views, reindex (reindex,reindex_parallel,new_empty) loads.- The
add()no-op is safe for the current query:results.push((*nid, sim))happens before the add atmain.rs:563, and the add was alreadylet _ =. - Chunk-loop termination holds. A non-empty chunk always yields at least one batch range,
choose_workersfloors at 1, and every worker commits its tail before the join, so each pass strictly shrinks theNOT EXISTSset. Rows that fail row-mapping are dropped byfilter_mapbut stay pending, and the loop still terminates because the final SELECT yields an all-bad chunk that maps to empty and breaks. - Cancel still works across the restructure:
cancelledis shared across chunks and checked at the loop head. - No SQL spacing bug from the new
count_sql, because the sequentialphase_clausecarries a trailing space where apartition_clausecan follow it.
One blocking issue though, and it is a direct consequence of moving the serving path to mmap. Details inline on src/index.rs.
Two process notes:
- The description covers only the reload fix and the ORT thread caps, but the PR also contains mmap-backed serving (item 4) and the chunked reindex (item 7), which are the two largest and riskiest changes in it. Worth folding into the body so the next reader knows the serving path and the reindex loop both changed shape.
- "Local compilation is not possible on the development machine" plus a test relaxed to non-fatal on Windows (
b6cc49f2) means CI is carrying the whole verification burden for a change that alters process memory semantics. The blocking issue below is one CI cannot catch, since no test runs a reindex and a serving daemon against the same index file concurrently.
Review follow-ups on #18: - build_from_db saved onto the live index path in place. Harmless when the daemon served a load() copy, but the daemon now holds an mmap view of exactly that inode: truncate-and-rewrite underneath the mapping is a SIGBUS (page past the truncated length) or torn reads for the whole duration of the write. It now writes a sibling tmp and renames, the same protocol as VecIndex::save - the mapping keeps the old inode until the next mtime-triggered re-view. - add() on a viewed handle is a silent Ok no-op, which the reindex flush path would misread as vector indexed, permanently dropping vectors from the index while their embed.db rows land. All reindex constructors produce writable handles; a debug_assert plus is_viewed() makes that invariant checked rather than conventional. - The lazy-embed site now documents the stated trade: on Linux/macOS a lazily-embedded node is not ANN-retrievable until the next reindex (it keeps taking the FTS+cosine path); durable in embed.db, so a recall-latency trade, not a correctness one.
raj-rkv
left a comment
raj-rkv
left a comment
There was a problem hiding this comment.
All three review points are addressed in 11f7dfd4, and I checked the fixes rather than the commit message.
The blocking one is properly fixed. build_from_db now writes <path>.usearch.tmp and renames onto the live path, the same protocol VecIndex::save already used. The tmp is a sibling, so the rename is atomic, and it keeps the old inode alive for a serving daemon's existing mapping until its next mtime-triggered re-view. It is also crash-safe: dying between the save and the rename leaves a stale tmp and an intact index. I checked you caught every in-place save rather than just the one I named, both inner.save sites in index.rs now target a tmp and nothing saves onto a live path any more.
is_viewed() plus the debug_assert lands in the right place, and I verified it covers the whole surface rather than the one site I quoted: there are exactly two .add( callers (main.rs:571 lazy-embed, :1127 flush_buffer), all four reindex handle constructors feed the asserted binding, and reindex_parallel holds no index handle at all, so one assert is genuinely enough. Worth being aware it is a debug_assert and so compiles out of release builds, which I think is the right call here since which constructor runs is statically determined and CI exercises the debug profile, but it does mean the guard is a development-time net rather than a production one.
The lazy-embed trade is now stated both at the call site and as its own section in the description, which is what makes it a decision rather than a side effect.
CI is green across the matrix (Linux and macOS default plus ort, Windows MSVC, MSRV 1.91, cargo-deny).
One non-blocking leftover for whenever the description is next touched: ## Changes still lists only the reload fix and the ORT thread caps. The new behaviour-change section covers mmap serving implicitly, but the chunked reindex (item 7) is still undocumented there, and it is roughly 200 lines restructuring both reindex paths into a chunked loop. The code itself I verified last round (termination holds, cancel propagates across chunks, every chunk commits before the next SELECT), so this is purely so the next reader of the description knows the reindex loop changed shape.
2 participants
Sidecar-side fixes from the memory RCA in Travsr-com/travsr#736 (items 4 and 5 of its fix plan).
Changes
HNSW reload: free the old index before loading the new one (C2)
VecIndex::knn's mtime-triggered reload did*self = Self::try_load(...), which built the replacement index while the old one was still alive: a transient 2x of the index's full size, at exactly the moment a just-finished reindex had already elevated memory. On a 264k-node repo that spike is hundreds of MB. The reload now resetsinnerto an empty index first, then loads into it. If the load fails, the index is empty rather than stale, and the nextknn()retries becauselast_modifiedwas not advanced.ORT session: explicit thread caps (C4/D)
The session builder set no thread configuration, so ONNX Runtime sized its intra-op pool from the host's physical core count via its own topology probe, which is not cgroup or quota aware. A container limited to 2 CPUs on a 64-core host got about 64 spinning threads and permanent throttling. The builder now sets:
with_intra_threads(available_parallelism()), which respects cgroup CPU quotas on Linux (Rust 1.64+) and returns the correct count on bare macOS and Windowswith_inter_threads(1), matching the sequential execution mode already in useTesting
with_intra_threads/with_inter_threadscalls were verified against the vendored ort 2.0.0-rc.12 source (BuilderResult = Result<SessionBuilder, Error<SessionBuilder>>, so the existingort_errmapping applies).Behaviour change (Linux/macOS)
With the serving path on an mmap view, the live HNSW is immutable between reindexes: a lazily-embedded node is no longer added to the in-memory index at query time. Its vector is still persisted to embed.db and the current query still scores and returns it (the FTS + direct-cosine lazy path), but it is not ANN-retrievable until the next reindex rebuilds the file and the daemon re-views. On a churning repo those nodes take the slower lazy path between reindexes. This is a deliberate trade for the memory win, not a side effect; Windows keeps the previous mutable-copy behaviour.
build_from_dbnow publishes via write-to-tmp + rename so a rebuild never rewrites the inode a serving daemon has mapped.