Conversation
…ws_per_key gate Fixes Fable's review of the merged store-backend change (#4270). Fix-forward on a new branch; the merged commit is untouched. P2a -- mmap the .idx instead of slurping it into heap. The in-memory index was a whole-file std::string (idx_blob_): non-evictable ANONYMOUS heap, and the .idx is 39-97% of the store. Since auto routes >2x-RAM stores to indexed, under that pressure indexed tried to allocate ~1x MemAvailable of non-evictable heap -> bad_alloc -> caught by step_execution as goal failure -> SILENT under-answering. Now .idx is mmap'd (PROT_READ, MAP_PRIVATE) exactly like .data: page-cache-backed + evictable, in-memory binary search reads through idx_base_/idx_len_ (bounds- checked idx_u32/idx_u16), ifstream slurp kept only as the non-POSIX fallback. Also: the mmap path charges nothing to the D43 counters at open (was charging the full index size); the false "small and constant" comment is corrected; munmap + fd close in the destructor (close_indexed()). P2b -- auto "lmdb usable" probe now matches what the C++ build needs: not just that the npm module loads, but that system liblmdb links (#include <lmdb.h> + -llmdb) and, best-effort, that an already-built lmdb store under DIR/lmdb/* actually mdb_env_opens (catches MDB_INVALID). Falls back to indexed on any failure; message reworded to what is probed. rows_per_key gate -- auto now picks lmdb only when store>2xRAM AND rows_per_key >= UW_STORE_LMDB_MIN_ROWS_PER_KEY (default 2), computed cheaply from the UWIX .idx headers (n_records/n_keys, no scan). A ~1-row/key store (ABI symprov = 1.03) stays on indexed even when huge (lmdb buys nothing). Unknown rpk (pre- build, no .idx) skips the gate (size-only). P3 + nits: guard tellg()<0 in ensure_open (was resize(SIZE_MAX) on a failed retry) and close streams/mmaps on throw paths (close_indexed); close data_fd_/idx_fd_ right after a successful mmap; bench_crossover.sh now ASSERTS rows_found identical across backends per cell (was print-only); validate UW_STORE_LMDB_RAM_FACTOR is an integer (else warn+default 2); test_auto_select.sh stubs the lmdb-usable probe so it can never trigger a real npm install, and adds rows_per_key gate cases; noted stat/od are GNU/coreutils. Guardrails: 503 differential + 51 corpus + 122 ABI verify all 0 (built -O0/nice, one at a time); goldens re-baselined (plain 96529->99808, lmdb 96770->100049; +3279 each, gate-independent); mmap-indexed rows_found == lmdb on a scale-store cross-check; test_auto_select ALL PASS (incl rpk=1->indexed, rpk=3->lmdb). Frozen resolver/store/debian untouched. Docs updated (BACKEND_SELECTION.md: rule now with rows_per_key + real probe, storage note on the mmap fix, key-sort deferred). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXjhStCqoig6944pVNBGe
…cold evict Second-reviewer residuals on PR #4271. 1. idx_key_compare: it formed `const unsigned char* k = idx_base_ + off` even when off > idx_len_ (a past-the-end pointer = UB, though never dereferenced), and an empty target with off>size fell through to `return 0` -- a spurious "match" yielding garbage offsets that read_record then bounds-rejects (dropped rows). Now handle out-of-range off BEFORE forming the pointer: if (off > idx_len_) return target.empty() ? 1 : -1; if (off + len > idx_len_) len = idx_len_ - off; Only reachable on a CORRUPT .idx; well-formed stores unaffected (503 differential + 51 corpus: 0 divergences). Goldens re-baselined (plain 99808->100151, lmdb 100049->100392; +343 each, gate-independent). 2. bench_main.cpp evict_file: it only called posix_fadvise(DONTNEED), a no-op on non-resident pages, so the "cold" numbers were cold-in-name-only. Now pre-faults the file (streaming read) BEFORE DONTNEED so eviction has resident pages to drop -- the cold lookups genuinely fault from disk. (bench-only; no goldens/gates.) Frozen resolver/store/debian untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RoXjhStCqoig6944pVNBGe
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix-forward for Fable's review of the merged store-backend change (#4270). New branch off current main; the merged commit is untouched. Do not merge (hub-gated; a kimi review is pending).
Findings addressed
.idxinstead of slurping it into heap. The in-memory index was a whole-filestd::string(non-evictable anonymous heap), and the.idxis 39-97% of the store. Becauseautoroutes>2×RAMstores to indexed, under that pressure indexed tried to allocate ~1× MemAvailable of non-evictable heap →bad_alloc→ caught as goal failure → silent under-answering. Now.idxismmap'd (PROT_READ, MAP_PRIVATE) like.data: page-cache-backed + evictable; binary search reads throughidx_base_/idx_len_(bounds-checkedidx_u32/idx_u16);ifstreamslurp kept as the non-POSIX fallback. Also stops charging the full index size to the D43 counters at open, and corrects the false "small and constant" comment.liblmdblinks (#include <lmdb.h>+-llmdb) and best-effortmdb_env_opens an already-built store (catchesMDB_INVALID), not just that the npm module loads. Falls back to indexed on any failure.autopicks lmdb only whenstore > 2×RAMandrows_per_key ≥ 2(from the.idxheader,n_records/n_keys, no scan). ABI symprov (1.03 rows/key) stays on indexed even when huge. Unknown rpk (pre-build) → size-only.tellg()<0inensure_open(wasresize(SIZE_MAX)on a failed retry) + close streams/maps on throw (close_indexed); close fds right after mmap;bench_crossover.shnow assertsrows_foundidentical across backends; validateUW_STORE_LMDB_RAM_FACTORis an integer;test_auto_select.shstubs the lmdb probe (no realnpm install) and adds rows_per_key cases; notedstat/odare GNU.Guardrails (all green)
-O0/nice, one at a time under memory pressure).plain 96529→99808,lmdb 96770→100049; +3279 each, gate-independent);test_wam_cpp_templatesgreen.rows_found == lmdbon a scale-store cross-check;test_auto_select.shALL PASS (inclrpk=1→indexed,rpk=3→lmdb).resolver.pl/resolver_store.pl/debian/untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01RoXjhStCqoig6944pVNBGe