diff --git a/Cargo.lock b/Cargo.lock index 2924cc0..2e37a4e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -783,25 +783,6 @@ dependencies = [ "crossbeam-utils", ] -[[package]] -name = "crossbeam-deque" -version = "0.8.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9dd111b7b7f7d55b72c0a6ae361660ee5853c9af73f70c3c2ef6858b950e2e51" -dependencies = [ - "crossbeam-epoch", - "crossbeam-utils", -] - -[[package]] -name = "crossbeam-epoch" -version = "0.9.18" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" -dependencies = [ - "crossbeam-utils", -] - [[package]] name = "crossbeam-utils" version = "0.8.20" @@ -869,15 +850,6 @@ dependencies = [ "zeroize", ] -[[package]] -name = "deranged" -version = "0.5.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d630bccd429a5bb5a64b5e94f693bfc48c9f8566418fda4c494cc94f911f87cc" -dependencies = [ - "powerfmt", -] - [[package]] name = "digest" version = "0.10.7" @@ -1099,12 +1071,6 @@ dependencies = [ "winit", ] -[[package]] -name = "either" -version = "1.15.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48c757948c5ede0e46177b7add2e67155f70e33c07fea8284df6576da70b3719" - [[package]] name = "emath" version = "0.32.3" @@ -2137,12 +2103,6 @@ dependencies = [ "vcpkg", ] -[[package]] -name = "linked-hash-map" -version = "0.5.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0717cef1bc8b636c6e1c1bbdefc09e6322da8a9321966e8928ef80d20f7f770f" - [[package]] name = "linux-raw-sys" version = "0.4.14" @@ -2209,25 +2169,6 @@ version = "0.4.33" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0ceec5bc11778974d1bcb055b18002eba7f4b3518b6a0081b3af5f21666da9ad" -[[package]] -name = "lopdf" -version = "0.32.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e775e4ee264e8a87d50a9efef7b67b4aa988cf94e75630859875fc347e6c872b" -dependencies = [ - "chrono", - "encoding_rs", - "flate2", - "itoa", - "linked-hash-map", - "log", - "md5", - "nom 7.1.3", - "rayon", - "time", - "weezl", -] - [[package]] name = "lopdf" version = "0.42.0" @@ -2245,7 +2186,7 @@ dependencies = [ "itoa", "log", "md-5", - "nom 8.0.0", + "nom", "rand 0.10.2", "rangemap", "sha2", @@ -2265,12 +2206,6 @@ dependencies = [ "digest", ] -[[package]] -name = "md5" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "490cc448043f947bae3cbee9c203358d62dbee0db12107a74be5c30ccfd09771" - [[package]] name = "memchr" version = "2.7.4" @@ -2323,12 +2258,6 @@ dependencies = [ "unicase", ] -[[package]] -name = "minimal-lexical" -version = "0.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "68354c5c6bd36d73ff3feceb05efa59b6acb7626617f4962be322a825e61f79a" - [[package]] name = "miniz_oxide" version = "0.8.9" @@ -2452,16 +2381,6 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2bf50223579dc7cdcfb3bfcacf7069ff68243f8c363f62ffa99cf000a6b9c451" -[[package]] -name = "nom" -version = "7.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d273983c5a657a70a3e8f2a01329822f3b8c8172b73826411a55751e404a0a4a" -dependencies = [ - "memchr", - "minimal-lexical", -] - [[package]] name = "nom" version = "8.0.0" @@ -2490,12 +2409,6 @@ dependencies = [ "windows-sys 0.48.0", ] -[[package]] -name = "num-conv" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "51d515d32fb182ee37cda2ccdcb92950d6a3c2893aa280e540671c2cd0f3b1d9" - [[package]] name = "num-traits" version = "0.2.19" @@ -2959,7 +2872,7 @@ dependencies = [ "encoding_rs", "euclid", "log", - "lopdf 0.42.0", + "lopdf", "postscript", "type1-encoding-parser", "unicode-normalization", @@ -3122,12 +3035,6 @@ version = "0.14.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "78451badbdaebaf17f053fd9152b3ffb33b516104eacb45e7864aaa9c712f306" -[[package]] -name = "powerfmt" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "439ee305def115ba05938db6eb1644ff94165c5ab5e9420d1c1bcedbba909391" - [[package]] name = "ppv-lite86" version = "0.2.17" @@ -3190,7 +3097,7 @@ dependencies = [ [[package]] name = "quicksearch-core" -version = "1.0.2" +version = "1.0.3" dependencies = [ "argon2", "cfb", @@ -3203,7 +3110,6 @@ dependencies = [ "kamadak-exif", "libc", "lofty", - "lopdf 0.32.0", "mime_guess", "notify", "pdf-extract", @@ -3223,7 +3129,7 @@ dependencies = [ [[package]] name = "quicksearch-gui" -version = "1.0.2" +version = "1.0.3" dependencies = [ "chrono", "eframe", @@ -3332,26 +3238,6 @@ version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "20675572f6f24e9e76ef639bc5552774ed45f1c30e2951e1e99c59888861c539" -[[package]] -name = "rayon" -version = "1.12.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fb39b166781f92d482534ef4b4b1b2568f42613b53e5b6c160e24cfbfa30926d" -dependencies = [ - "either", - "rayon-core", -] - -[[package]] -name = "rayon-core" -version = "1.13.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22e18b0f0062d30d4230b2e85ff77fdfe4326feb054b9783a3460d8435c8ab91" -dependencies = [ - "crossbeam-deque", - "crossbeam-utils", -] - [[package]] name = "redox_syscall" version = "0.4.1" @@ -3957,36 +3843,6 @@ dependencies = [ "zune-jpeg", ] -[[package]] -name = "time" -version = "0.3.43" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83bde6f1ec10e72d583d91623c939f623002284ef622b87de38cfd546cbf2031" -dependencies = [ - "deranged", - "num-conv", - "powerfmt", - "serde", - "time-core", - "time-macros", -] - -[[package]] -name = "time-core" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "40868e7c1d2f0b8d73e4a8c7f0ff63af4f6d19be117e90bd73eb1d62cf831c6b" - -[[package]] -name = "time-macros" -version = "0.2.24" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "30cfb0125f12d9c277f35663a0a33f8c30190f4e4574868a330595412d34ebf3" -dependencies = [ - "num-conv", - "time-core", -] - [[package]] name = "tinystr" version = "0.7.6" diff --git a/Cargo.toml b/Cargo.toml index 045b6e3..0bd6d53 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ members = [ ] [workspace.package] -version = "1.0.2" +version = "1.0.3" edition = "2021" license = "GPL-3.0-or-later" authors = ["Jeremy "] diff --git a/README.md b/README.md index 3cb1ccb..fb7085f 100644 --- a/README.md +++ b/README.md @@ -389,14 +389,25 @@ Synchronous Rust: `std::thread` + `mpsc` channels, no async runtime. open applies the Argon2id-derived raw key (`security.rs`, process-global in `db/key.rs`) before anything reads the file; a wrong key is a tagged `KEY_MISMATCH` error, structurally distinct from the schema drift that - may wipe, so it can never destroy an intact index. + may wipe, so it can never destroy an intact index. Each kind of connection + takes a page cache sized for what it does and how long it lives, rather than + one figure applied everywhere (`db/schema.rs` sets six profiles and argues + each): the caches are `malloc`ed, so a connection that scans a table and is + then held — the coordinator's writer, before it learned to let go when idle — + keeps that memory for the life of the process. Search is the one deliberately + large one, because it is the only cache reused often enough to pay for + itself, and it is released once searching stops. - **Indexing** (`indexing.rs`, `file_handling.rs`): full runs walk each root (`filtered_walk` prunes hidden/ignored subtrees before descending), classify files by mtime into insert/update/skip, batch-write metadata, sweep stale rows, then extract content (plaintext, RTF, Office — both the OOXML/ODF zip formats and the pre-2007 binary `.doc`/`.xls`/`.ppt`, whose OLE2 streams are read in `extract/ole.rs` — PDF, audio tags, EXIF; see - `extract/`) for FTS. Files whose extension no MIME + `extract/`) for FTS. PDFs are parsed once, with the text and the `Info` + dictionary taken off the same document: the two-parse version that preceded + it was a run's largest single memory consumer, and it was what pulled a + second copy of `lopdf` — and with it rayon's never-torn-down thread pool — + into the build. Files whose extension no MIME table knows — including extensionless ones like `README` or `Makefile` — are sniffed from their head bytes and indexed as text only when that head is provably text: valid UTF-8, or BOM-marked (`mime.rs`, `textenc.rs`). @@ -466,7 +477,11 @@ Synchronous Rust: `std::thread` + `mpsc` channels, no async runtime. - **Search** (`search/`): `SearchService` runs one worker thread; each query is a *generation*. New queries interrupt the in-flight SQLite statement (`InterruptHandle`) and stale generations stop cooperatively, - so typing never waits. The cascade streams rank-ordered batches: one + so typing never waits. The worker keeps its connection across requests and + drops it once searching stops, so a typing session runs against a page cache + that is already warm instead of rebuilding one per keystroke; because a + rebuild or clear puts a *new* file at the *same* path, an index generation + counter (`db::index_epoch`) is what tells the held connection to reopen. The cascade streams rank-ordered batches: one `files` scan classifies exact/case/substring filename matches (ranks 1–4) and, since a path contains its own name, sets aside full-path matches from the same rows (ranks 9–10); one FTS phrase probe verified @@ -533,6 +548,24 @@ pagination: the table is virtualized, so a single scroll list capped at and duplicates tabs, and query highlighting. - `QSB_SNIPPET_PERF=1 cargo test --release -p quicksearch-core --test snippet_perf -- --nocapture`: snippet pipeline benchmark. +- `QSB_SEARCH_PERF=1 cargo test --release -p quicksearch-core --test + search_perf -- --nocapture`: what a warm page cache is worth to search, swept + across cache ceilings and run both encrypted and not. It exists because the + right size for `PRAGMAS_SEARCH` is not something to reason about: encrypted, + the curve has a cliff at the working set, because SQLCipher caches pages + decrypted and a miss below that costs an AES-CBC plus an HMAC-SHA512 per + 4 KiB page. Unencrypted it is flat. Read it before changing that number. +- Memory probes, all under `crates/quicksearch-core/examples/`: + `memprobe ` reports an indexing run's peak *and* what + it settles at once idle — the gap between those is the memory a process + keeps for nothing, since glibc's `free` returns chunks to its arena rather + than to the kernel. `rssprobe [duration_s]` attributes a *running* + process's footprint instead, splitting anonymous heap (ours) from + file-backed pages (the binary, libc, the GL stack), reading `Private_Dirty` + rather than `VmRSS`, and counting glibc's arenas so retention is + distinguishable from live data. It reads another process's `/proc`, so it + measures a build made without knowing it would be measured. + `indexprobe` and `walkprobe` answer "how fast" rather than "how much". - `.forgejo/workflows/ci.yml`: builds both platforms on every push to `master` and every pull request. To cut a release, bump `[workspace.package] version` in `Cargo.toml` and push the commit on a branch named `Release...`; CI runs diff --git a/crates/quicksearch-core/Cargo.toml b/crates/quicksearch-core/Cargo.toml index 6e2e260..026d209 100644 --- a/crates/quicksearch-core/Cargo.toml +++ b/crates/quicksearch-core/Cargo.toml @@ -49,8 +49,12 @@ chardetng = "1.0" # RTF text extraction. Pure Rust; with the default `jsbindings` feature off # (it exists for the crate's WASM build) it depends only on serde. rtf-parser = { version = "0.4", default-features = false } +# `lopdf` is deliberately NOT declared here. `pdf-extract` re-exports it +# (`pub use lopdf::*`), and `extract/pdf.rs` reaches it that way. Naming it +# directly resolves a *second*, older copy alongside pdf-extract's — which is +# what used to make every PDF parse twice, and what pulled in rayon (whose +# global thread pool is never torn down), chrono, time, md5 and a second nom. pdf-extract = "0.12" -lopdf = "0.32" lofty = "0.19" kamadak-exif = "0.5" notify = "6.1" diff --git a/crates/quicksearch-core/examples/memprobe.rs b/crates/quicksearch-core/examples/memprobe.rs index 86ad400..25de989 100644 --- a/crates/quicksearch-core/examples/memprobe.rs +++ b/crates/quicksearch-core/examples/memprobe.rs @@ -52,13 +52,29 @@ //! peak to the walk or to extraction; a sampled peak far under VmHWM means //! the real spike was shorter than the sampling interval. //! -//! Nothing here is evictable page cache. The connection sets no `mmap_size`, -//! so SQLite reads the index through its own `malloc`'d page cache (`PRAGMA -//! cache_size`, 10000 pages ≈ 40 MiB) rather than mapping the file, and the -//! `by mapping` breakdown confirms it: the index never appears as a -//! file-backed mapping. Every megabyte reported is memory the process -//! actually holds. `/usr/bin/time -v` on this binary reports the same VmHWM, -//! as a cross-check that nothing here is fooling itself. +//! Nothing here is evictable page cache. The connections set no `mmap_size`, +//! so SQLite reads the index through its own `malloc`'d page cache (see +//! [`quicksearch_core::db::schema`], where every profile's ceiling is set and +//! argued) rather than mapping the file, and the `by mapping` breakdown +//! confirms it: the index never appears as a file-backed mapping. Every +//! megabyte reported is memory the process actually holds. `/usr/bin/time -v` +//! on this binary reports the same VmHWM, as a cross-check that nothing here +//! is fooling itself. +//! +//! # Peak, settled, and the difference between them +//! +//! `settled RSS` is read once the run is over and the service is idle, and it +//! is the number that matters for a process that stays open. It is not the +//! peak minus the run's buffers: glibc's `free` returns a chunk to its arena +//! rather than to the kernel, so without an explicit +//! [`quicksearch_core::platform::release_free_heap`] a run's high-water +//! becomes the process's floor for as long as it lives. On a 65k-file tree +//! that was the difference between settling at 89 MiB and settling at 34 MiB. +//! +//! For the *idle* footprint of a running GUI — which includes the window, the +//! GL stack and everything this binary deliberately excludes — use +//! [`rssprobe`](rssprobe.rs), which reads another process's `/proc` and so can +//! measure a build that was made without knowing it would be measured. use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; @@ -80,6 +96,18 @@ const MARKER_INTERVAL_MS: u64 = 500; /// be worse than failing. const TIMEOUT: Duration = Duration::from_secs(3 * 3600); +/// How long to wait for the run to reach `Idle` before measuring what it left +/// behind. Generous because the wait is for `VACUUM` on a multi-gigabyte index +/// ([`quicksearch_core::db::open`]'s maintenance connection), not for anything +/// that scales with the sampling interval. +const SETTLE_TIMEOUT: Duration = Duration::from_secs(600); + +/// The pause after `Idle` is published. The writer thread releases its +/// connection and returns free pages to the kernel *after* setting the status, +/// so sampling the instant it flips would miss exactly the thing being +/// measured. +const SETTLE_QUIET: Duration = Duration::from_secs(2); + /// Resident bytes at the peak, grouped by what the mapping is. /// /// A peak figure alone cannot be acted on: 60 MiB of heap is a buffer to @@ -222,13 +250,40 @@ fn run(mode: &str, root: &str, db: &Path, interval: Duration, config_path: Optio // nothing that VmHWM would forget anyway. let hwm = vm_hwm(); assert!(done, "indexing did not finish within {:?}", TIMEOUT); + + let settled = settle(&service); service.stop_indexing().expect("stop"); report( - mode, elapsed, baseline, hwm, &samples, db, interval, &at_peak, + mode, elapsed, baseline, hwm, settled, &samples, db, interval, &at_peak, ); } +/// RSS once the run has fully finished and stopped allocating. +/// +/// The peak says what indexing needs; this says what it *keeps*, and the gap +/// between them is the number that decides whether an idle QuickSearch is +/// holding memory it has no use for. They differ by more than the run's own +/// buffers: `free` under glibc returns a chunk to its arena rather than to the +/// kernel, so what is measured here is the floor the process will sit at for +/// as long as it stays open. +/// +/// The marker this loop's caller watches for is written *during* the run, so +/// maintenance and the final release are still ahead of it — hence waiting for +/// `Idle` rather than sampling immediately, and then a little longer, because +/// the last of it happens after the status is published. +fn settle(service: &IndexingService) -> u64 { + let deadline = Instant::now() + SETTLE_TIMEOUT; + while Instant::now() < deadline { + if matches!(service.get_status(), IndexingStatus::Idle) { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + std::thread::sleep(SETTLE_QUIET); + rss().unwrap_or(0) +} + /// Flatten per-root progress into one line's worth of numbers. Roots are /// summed: the process has one address space, so a per-root split would not /// explain a peak that several roots contribute to at once. @@ -262,6 +317,7 @@ fn report( elapsed: Duration, baseline: u64, hwm: Option, + settled: u64, samples: &[Sample], db: &Path, interval: Duration, @@ -316,6 +372,17 @@ fn report( ); } eprintln!(" baseline RSS {}", mib(baseline)); + eprintln!( + " settled RSS {} (once idle: what the run kept, not what it needed)", + mib(settled) + ); + if let Some(h) = hwm { + eprintln!( + " returned {} of the {} the run took above baseline", + mib(h.saturating_sub(settled)), + mib(h.saturating_sub(baseline)) + ); + } if let Some(h) = hwm { if files > 0 { eprintln!( diff --git a/crates/quicksearch-core/examples/rssprobe.rs b/crates/quicksearch-core/examples/rssprobe.rs new file mode 100644 index 0000000..e3f8ca6 --- /dev/null +++ b/crates/quicksearch-core/examples/rssprobe.rs @@ -0,0 +1,442 @@ +//! Steady-state memory accounting for an already-running process. +//! +//! [`memprobe`](memprobe.rs) answers "how much RAM does a run peak at"; this +//! answers "how much is the process still holding once it has nothing to do", +//! which is a different question with a different answer. Under glibc a peak +//! is not returned to the OS when it is freed, so the idle floor is set by the +//! largest thing that ever happened rather than by anything currently live. +//! Telling those apart is the whole point of this probe. +//! +//! It reads another process's `/proc` rather than its own, so it measures a +//! binary that was built without knowing it would be measured. That matters +//! for a before/after: an in-process diagnostic would mean the "before" and +//! "after" numbers come from different binaries, and the delta would include +//! the diagnostic itself. +//! +//! ```text +//! cargo build -p quicksearch-core --example rssprobe --release +//! ./target/release/examples/rssprobe $(pgrep -x quicksearch) +//! ./target/release/examples/rssprobe $(pgrep -x quicksearch) 60 +//! ./target/release/examples/rssprobe $(pgrep -x quicksearch) 60 250 +//! ``` +//! +//! With no duration it takes one snapshot and prints the full breakdown. With +//! one it samples for that many seconds first, so a number can be quoted for +//! an *idle* process rather than for whatever the process happened to be doing +//! the instant the probe ran. +//! +//! **`VmRSS` is the wrong number to optimise and it is the one every system +//! monitor shows.** Most of it here is `Shared_Clean`: the binary's own text, +//! libc, and the Mesa/GL stack the window pulls in. Those pages are shared +//! with every other process using them and the kernel drops them under +//! pressure. The two numbers worth acting on are: +//! +//! - **`RssAnon`** — heap and thread stacks. Nothing else. This is the share +//! this codebase allocates and can therefore give back. +//! - **`Private_Dirty`** — what the process actually costs the machine, from +//! `smaps_rollup`. Nobody else is sharing it and it cannot be evicted, only +//! freed. +//! +//! The `glibc arenas` line is the one that says whether an anonymous figure is +//! live data or retention. glibc allocates each non-main arena as a 64 MiB +//! aligned region and commits into it; it never unmaps one, and `free` returns +//! chunks to the arena rather than to the kernel. So a stack of 64 MiB regions +//! holding far less than 64 MiB each is freed memory the process is still +//! charged for — which `malloc_trim(3)` can return and dropping a buffer +//! cannot. Live data does not look like that. + +use std::collections::HashMap; +use std::path::PathBuf; +use std::time::{Duration, Instant}; + +/// Default gap between samples. One `status` plus one `smaps_rollup` read per +/// sample, both small; the interval is set by how fine the timeline should be +/// rather than by overhead. `smaps` is read once at the end, not per sample — +/// it is the expensive one, being proportional to the mapping count. +const DEFAULT_SAMPLE_MS: u64 = 500; + +/// The size glibc reserves per non-main arena (`HEAP_MAX_SIZE` on 64-bit). +/// Regions of exactly this size, aligned to it, are arenas rather than +/// anything the program asked for. +const ARENA_SPAN: u64 = 64 * 1024 * 1024; + +/// One sample: the cheap per-interval reads only. +struct Sample { + at: Duration, + rss: u64, + anon: u64, + file: u64, + private_dirty: u64, + pss: u64, + /// Carried per sample rather than read once at the end, so it survives the + /// process exiting mid-run — it comes from the same `status` read anyway. + hwm: u64, +} + +/// Resident bytes per mapping, plus the two groupings that answer the +/// retention question directly. +#[derive(Default)] +struct Breakdown { + /// Mapping name (or anonymous size class) to resident bytes, descending. + entries: Vec<(String, u64)>, + /// Non-main glibc arenas: how many, and resident across all of them. + arenas: (usize, u64), + /// The main arena, which grows by `brk` and is the one the kernel labels. + main_heap: u64, +} + +fn main() { + let mut args = std::env::args().skip(1); + let Some(pid) = args.next().and_then(|p| p.parse::().ok()) else { + eprintln!("usage: rssprobe [duration_s] [interval_ms]"); + std::process::exit(2); + }; + let duration = Duration::from_secs( + args.next() + .map(|s| s.parse().expect("duration_s must be a number")) + .unwrap_or(0), + ); + let interval = Duration::from_millis( + args.next() + .map(|s| s.parse().expect("interval_ms must be a number")) + .unwrap_or(DEFAULT_SAMPLE_MS) + .max(1), + ); + + if !PathBuf::from(format!("/proc/{}", pid)).exists() { + eprintln!("rssprobe: no process {}", pid); + std::process::exit(1); + } + eprintln!("rssprobe pid={} {}", pid, comm(pid)); + + let (samples, at_end) = sample_for(pid, duration, interval); + report(&samples, &at_end, interval); +} + +/// Sample until `duration` elapses. A zero duration still takes one sample, +/// so the no-argument form is a snapshot rather than an error. +/// +/// The per-mapping breakdown is taken alongside the last *live* sample rather +/// than after the loop, because a process that exits during a long run would +/// otherwise report an empty one — which reads like "nothing was resident" +/// rather than "nobody was there to ask". +fn sample_for(pid: u32, duration: Duration, interval: Duration) -> (Vec, Breakdown) { + let start = Instant::now(); + let mut samples = Vec::new(); + let mut at_end = Breakdown::default(); + loop { + let elapsed = start.elapsed(); + let Some(s) = sample(pid, elapsed) else { + if samples.is_empty() { + eprintln!("rssprobe: cannot read /proc/{}: process gone?", pid); + std::process::exit(1); + } + eprintln!( + " process exited after {:.1}s; figures below are its last live sample", + elapsed.as_secs_f64() + ); + break; + }; + samples.push(s); + // smaps is proportional to the mapping count, so it is read once per + // sample only when the sample might be the last one — which, until the + // loop ends, is every one of them. + at_end = breakdown(pid); + if start.elapsed() >= duration { + break; + } + std::thread::sleep(interval); + } + (samples, at_end) +} + +fn sample(pid: u32, at: Duration) -> Option { + let status = proc_kv(pid, "status")?; + // smaps_rollup is the kernel's own sum over smaps, so it costs one read + // rather than one per mapping. Missing only on kernels this old code will + // never meet; treat it as zero rather than failing the sample. + let rollup = proc_kv(pid, "smaps_rollup").unwrap_or_default(); + Some(Sample { + at, + rss: *status.get("VmRSS").unwrap_or(&0), + anon: *status.get("RssAnon").unwrap_or(&0), + file: *status.get("RssFile").unwrap_or(&0), + private_dirty: *rollup.get("Private_Dirty").unwrap_or(&0), + pss: *rollup.get("Pss").unwrap_or(&0), + hwm: *status.get("VmHWM").unwrap_or(&0), + }) +} + +/// Parse a `/proc//` of `Key: N kB` lines into bytes. +/// +/// Both `status` and `smaps_rollup` are in this format, and both carry lines +/// that are not sizes at all (`Name:`, `State:`, the rollup's address range). +/// Anything that does not end in `kB` is skipped rather than guessed at. +fn proc_kv(pid: u32, file: &str) -> Option> { + let text = std::fs::read_to_string(format!("/proc/{}/{}", pid, file)).ok()?; + let mut out = HashMap::new(); + for line in text.lines() { + let Some((key, rest)) = line.split_once(':') else { + continue; + }; + let mut fields = rest.split_whitespace(); + let (Some(value), Some("kB")) = (fields.next(), fields.next()) else { + continue; + }; + if let Ok(kib) = value.parse::() { + out.insert(key.to_string(), kib * 1024); + } + } + Some(out) +} + +fn comm(pid: u32) -> String { + std::fs::read_to_string(format!("/proc/{}/comm", pid)) + .map(|s| s.trim().to_string()) + .unwrap_or_default() +} + +/// Resident bytes per mapping from `/proc//smaps`, summed by name, with +/// glibc's arenas identified. +/// +/// The name is the mapping's path, or `[heap]`/`[stack]` for the ones the +/// kernel labels. Everything else is anonymous — thread stacks, arena heaps, +/// and any single `malloc` large enough to have gone to `mmap` — and is +/// bucketed by size class, because individually they are unnamed and there can +/// be hundreds of them. +fn breakdown(pid: u32) -> Breakdown { + let Ok(smaps) = std::fs::read_to_string(format!("/proc/{}/smaps", pid)) else { + return Breakdown::default(); + }; + + // Two passes over the same parse: one for the by-name totals the report + // prints, one to find arenas — which needs the raw address ranges the + // names throw away. + let mut by_name: HashMap = HashMap::new(); + let mut regions: Vec = Vec::new(); + let mut current = String::new(); + + for line in smaps.lines() { + if let Some(rss_kib) = line.strip_prefix("Rss:") { + let kib: u64 = rss_kib + .split_whitespace() + .next() + .and_then(|v| v.parse().ok()) + .unwrap_or(0); + *by_name.entry(current.clone()).or_default() += kib * 1024; + if let Some(r) = regions.last_mut() { + r.rss = kib * 1024; + } + } else if let Some(region) = parse_map_header(line) { + current = region.name.clone(); + regions.push(region); + } + } + + let mut entries: Vec<(String, u64)> = by_name.into_iter().filter(|(_, b)| *b > 0).collect(); + entries.sort_by_key(|(_, bytes)| std::cmp::Reverse(*bytes)); + + Breakdown { + entries, + arenas: find_arenas(®ions), + main_heap: regions + .iter() + .filter(|r| r.name == "[heap]") + .map(|r| r.rss) + .sum(), + } +} + +/// One `smaps` mapping, kept with its address range so arenas can be found. +struct Region { + lo: u64, + hi: u64, + anonymous: bool, + name: String, + rss: u64, +} + +/// Count glibc's non-main arenas and their resident bytes. +/// +/// An arena is one `mmap` of [`ARENA_SPAN`], aligned to it, which glibc then +/// commits into piecewise — so by the time it reaches `smaps` it has usually +/// been split into a readable part and a `---p` remainder. Matching a single +/// mapping of the full span therefore misses most of them. Contiguous runs of +/// anonymous mappings are coalesced first, and a run that spans exactly one +/// aligned [`ARENA_SPAN`] is an arena. +/// +/// This can in principle collide with a program that mmaps 64 MiB at 64 MiB +/// alignment on purpose. Nothing here does, and the alternative — parsing +/// glibc's internal `heap_info` out of the process — is not worth it for a +/// figure whose job is to point at a cause rather than to be exact. +fn find_arenas(regions: &[Region]) -> (usize, u64) { + let mut count = 0; + let mut resident = 0; + let mut i = 0; + while i < regions.len() { + if !regions[i].anonymous { + i += 1; + continue; + } + let lo = regions[i].lo; + let mut j = i; + let mut rss = 0; + while j < regions.len() + && regions[j].anonymous + && regions[j].lo == if j == i { lo } else { regions[j - 1].hi } + && regions[j].hi <= lo + ARENA_SPAN + { + rss += regions[j].rss; + j += 1; + } + if j > i && regions[j - 1].hi == lo + ARENA_SPAN && lo.is_multiple_of(ARENA_SPAN) { + count += 1; + resident += rss; + i = j; + } else { + i += 1; + } + } + (count, resident) +} + +/// Parse one `smaps` header line: `addr-addr perms offset dev inode [path]`. +/// +/// Anonymous mappings have no path; they are named by size class so that a +/// hundred 8 MiB regions read as one line rather than a hundred. +fn parse_map_header(line: &str) -> Option { + let mut fields = line.split_whitespace(); + let range = fields.next()?; + let (lo, hi) = range.split_once('-')?; + let lo = u64::from_str_radix(lo, 16).ok()?; + let hi = u64::from_str_radix(hi, 16).ok()?; + // Fields 2-5 are perms, offset, dev, inode; anything after is the path. + let path = fields.nth(4).unwrap_or(""); + Some(Region { + lo, + hi, + anonymous: path.is_empty(), + name: if path.is_empty() { + format!("anon {}", size_class(hi.saturating_sub(lo))) + } else { + path.to_string() + }, + rss: 0, + }) +} + +/// Power-of-two bucket, so mappings group by what allocated them rather than +/// by their exact size. +fn size_class(bytes: u64) -> String { + let mib = bytes as f64 / (1024.0 * 1024.0); + if mib < 1.0 { + "< 1 MiB".to_string() + } else { + let bucket = 1u64 << (63 - (bytes / (1024 * 1024)).leading_zeros() as u64); + format!("~{} MiB", bucket) + } +} + +fn report(samples: &[Sample], at_end: &Breakdown, interval: Duration) { + if samples.len() > 1 { + // One line per 5% of the run, so the shape is visible at any duration. + let step = (samples.len() / 20).max(1); + eprintln!( + "\n {:>8} {:>10} {:>10} {:>10} {:>12} {:>10}", + "t", "VmRSS", "RssAnon", "RssFile", "PrivDirty", "Pss" + ); + for s in samples.iter().step_by(step) { + eprintln!( + " {:>7.1}s {:>10} {:>10} {:>10} {:>12} {:>10}", + s.at.as_secs_f64(), + mib(s.rss), + mib(s.anon), + mib(s.file), + mib(s.private_dirty), + mib(s.pss) + ); + } + } + + let last = samples.last().expect("at least one sample"); + let elapsed = last.at; + eprintln!( + "\nsteady state over {:.1}s ({} samples, {:?} apart)", + elapsed.as_secs_f64(), + samples.len(), + interval + ); + + // Both ends of each range, because a figure that is drifting is a + // different finding from one that is flat, and only the flat one can be + // called a floor. + range_line("VmRSS", samples, |s| s.rss, ""); + range_line( + "RssAnon", + samples, + |s| s.anon, + "heap + thread stacks: the share this code owns", + ); + range_line( + "RssFile", + samples, + |s| s.file, + "binary text, libc, Mesa/GL: shared and evictable", + ); + range_line( + "Private_Dirty", + samples, + |s| s.private_dirty, + "what this process costs the machine", + ); + range_line("Pss", samples, |s| s.pss, ""); + + eprintln!( + " {:<16} {:>10} kernel high-water: the peak this floor was left by", + "VmHWM", + mib(last.hwm) + ); + + let b = at_end; + let (arenas, arena_rss) = b.arenas; + eprintln!( + "\n glibc heap: {} non-main arena(s) holding {}, plus [heap] {}", + arenas, + mib(arena_rss), + mib(b.main_heap) + ); + if arenas > 0 { + eprintln!( + " reserved {} across them, so {:.0}% of the arena space is resident", + mib(arenas as u64 * ARENA_SPAN), + 100.0 * arena_rss as f64 / (arenas as u64 * ARENA_SPAN) as f64 + ); + } + + if !b.entries.is_empty() { + eprintln!("\n resident bytes by mapping:"); + for (name, bytes) in b.entries.iter().take(12) { + eprintln!(" {:>10} {}", mib(*bytes), name); + } + } +} + +fn range_line(label: &str, samples: &[Sample], get: fn(&Sample) -> u64, note: &str) { + let last = get(samples.last().expect("at least one sample")); + let min = samples.iter().map(get).min().unwrap_or(0); + let max = samples.iter().map(get).max().unwrap_or(0); + let spread = if samples.len() > 1 && max > min { + format!(" (min {}, max {})", mib(min), mib(max)) + } else { + String::new() + }; + if note.is_empty() { + eprintln!(" {:<16} {:>10}{}", label, mib(last), spread); + } else { + eprintln!(" {:<16} {:>10} {}{}", label, mib(last), note, spread); + } +} + +fn mib(bytes: u64) -> String { + format!("{:.1} MiB", bytes as f64 / (1024.0 * 1024.0)) +} diff --git a/crates/quicksearch-core/src/cli.rs b/crates/quicksearch-core/src/cli.rs index a577df1..db2d5bb 100644 --- a/crates/quicksearch-core/src/cli.rs +++ b/crates/quicksearch-core/src/cli.rs @@ -8,16 +8,23 @@ //! //! # These are consumed from outside this repository //! -//! QuickSearch is a sub-repo. Of everything here only [`index_counts`] has a -//! caller in this tree (the GUI status bar); [`status_for_path`], -//! [`list_failed`], [`index_size_breakdown`], [`pending_content_count`] and -//! [`clear_path`] are called by the parent repository's Baloo compat daemon, -//! which is what reports them to `balooctl` and mirrors them into LMDB. +//! QuickSearch is a sub-repo, and **nothing in this module has a caller in +//! this tree**. [`status_for_path`], [`list_failed`], [`index_size_breakdown`], +//! [`pending_content_count`] and [`clear_path`] are called by the parent +//! repository's Baloo compat daemon, which is what reports them to `balooctl` +//! and mirrors them into LMDB. //! //! So they are **not dead code**, and their signatures are a compatibility //! surface rather than an internal detail: a search of this repository alone //! will not turn up the callers that break when one changes. //! +//! There used to be an `index_counts` here as well, for the GUI's status bar. +//! It ran three `COUNT(*)` scans — one of them unindexed — of which the +//! frontend displayed one, on a thread and a connection opened afresh every +//! five seconds. The surviving figure is published by +//! [`crate::coordinator::IndexerState::files`] instead, off the connection the +//! coordinator already has. +//! //! One exception to the "query helpers" framing: [`clear_path`] mutates. It //! opens its own writer, which sidesteps the single-writer discipline the //! coordinator maintains, so it is safe only against an index no local @@ -206,29 +213,6 @@ pub fn pending_content_count(db_path: &str) -> Result { .map_err(|e| format!("pending_content_count: {}", e)) } -/// Cheap aggregate counts for the GUI's idle status bar ("N files -/// indexed"). Callers cache the result; it's three COUNT scans, not -/// something to run per frame. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct IndexCounts { - pub files: i64, - pub content_done: i64, - pub content_pending: i64, -} - -pub fn index_counts(db_path: &str) -> Result { - let conn = open_existing(db_path, false)?; - let count = |sql: &str| -> Result { - conn.query_row(sql, [], |r| r.get(0)) - .map_err(|e| format!("index_counts: {}", e)) - }; - Ok(IndexCounts { - files: count("SELECT COUNT(*) FROM files")?, - content_done: count("SELECT COUNT(*) FROM files WHERE content_state = 1")?, - content_pending: count("SELECT COUNT(*) FROM files WHERE content_state = 0")?, - }) -} - /// Remove a single file from the index. Returns whether a row was deleted. /// Keeps FTS/documents/properties in sync via the repo helpers. pub fn clear_path(db_path: &str, path: &str) -> Result { diff --git a/crates/quicksearch-core/src/content.rs b/crates/quicksearch-core/src/content.rs index 136e187..91c8004 100644 --- a/crates/quicksearch-core/src/content.rs +++ b/crates/quicksearch-core/src/content.rs @@ -23,7 +23,7 @@ use std::sync::atomic::AtomicBool; use std::sync::{mpsc, Arc, Condvar, Mutex}; -use std::thread::{self, JoinHandle}; +use std::thread::JoinHandle; use crate::config::Config; use crate::extract::Registry; @@ -311,7 +311,7 @@ pub fn extract_content( let (registry, config) = (registry.clone(), config.clone()); let (stop_flag, suspend_flag) = (stop_flag.clone(), suspend_flag.clone()); let stats = stats.clone(); - thread::spawn(move || { + crate::platform::spawn_worker("qs-extract", move || { crate::platform::set_background_priority(); worker( &shared, @@ -332,7 +332,7 @@ pub fn extract_content( let feeder_handle = { let (shared, db_path, cursor) = (shared.clone(), db_path.to_string(), cursor.clone()); - thread::spawn(move || { + crate::platform::spawn_worker("qs-feeder", move || { crate::platform::set_background_priority(); feeder(&shared, &db_path, cursor, max_size) }) @@ -350,6 +350,10 @@ pub fn extract_content( #[cfg(test)] mod tests { use super::*; + // Only the tests sleep; the pass itself spawns through + // `platform::spawn_worker` and blocks on channels rather than polling. + use std::thread; + use crate::db::open_or_recreate; use crate::db::repo::{self, insert_file, NewFile}; use crate::file_handling::{extract_scope_prepare, store_extracted}; diff --git a/crates/quicksearch-core/src/coordinator.rs b/crates/quicksearch-core/src/coordinator.rs index 0fa0130..75a930c 100644 --- a/crates/quicksearch-core/src/coordinator.rs +++ b/crates/quicksearch-core/src/coordinator.rs @@ -21,6 +21,18 @@ //! run is active — the coordinator's tick simply does nothing until the //! `IndexingService` reports idle, then drains its queue. Overflowing the //! queue (>100k pending paths) collapses into one full run instead. +//! +//! Settling is part of the state machine, not an implementation detail. The +//! first tick that finds nothing to do calls [`Inner::go_idle`], which drops +//! the write connection — reopening it is cheap, and holding it means holding +//! whatever page cache the last reconciliation filled for the life of the +//! process — and then returns the heap that this coordinator, the search +//! worker and the last indexing run have all freed into their allocator +//! arenas. It fires once per busy→idle transition, so a coordinator that has +//! nothing to do costs nothing to have around. This is also where the +//! published file count comes from: the status bar's figure is read off the +//! connection this thread already holds, on an interval, rather than by a +//! frontend opening its own. use std::collections::HashMap; use std::path::PathBuf; @@ -42,6 +54,15 @@ use crate::watcher::{FsEvent, WatchError, WatchFilters, Watcher, WatcherConfig}; /// Pending-event ceiling; beyond this a full run is cheaper than replay. const PENDING_OVERFLOW: usize = 100_000; +/// How often the published file count is re-read while idle. +/// +/// It moves only when something writes to the index, and everything that does +/// so — a run finishing, a batch of watcher events — either invalidates it +/// directly or is followed by another tick within this window. Long enough +/// that a `COUNT(*)` over a multi-million-row index is not a recurring cost, +/// short enough that the status bar is not visibly wrong. +const FILE_COUNT_INTERVAL: Duration = Duration::from_secs(30); + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum IndexMode { Auto, @@ -96,6 +117,12 @@ pub struct IndexerState { pub activity: IndexingStatus, /// Unix seconds of the last completed full run, if any. pub last_full_index: Option, + /// Rows in the index, refreshed while idle. `None` before the first read. + /// + /// Deliberately approximate: it is a status-bar figure, not a fact + /// anything decides on, and the alternative to letting it lag is running a + /// `COUNT(*)` on a cadence rather than when the index is quiet. + pub files: Option, /// Watcher events waiting to be applied. pub queued_events: usize, /// Live-update health; see [`WatcherStatus`]. @@ -164,6 +191,15 @@ impl ReconcileStop { struct Shared { mode: IndexMode, last_full_index: Option, + /// Rows in `files`, for the idle status bar's "N files indexed". + /// + /// Published from here rather than fetched by the frontend because the + /// coordinator is already the thing that holds a connection and already + /// knows when the index is quiet enough to ask. The GUI used to spawn a + /// thread and open its own connection for this every five seconds, which + /// cost a page cache and an arena per refresh for a decorative number. + /// `None` until the first successful read. + files: Option, queued_events: usize, watcher: WatcherStatus, reconcile: Option, @@ -192,6 +228,7 @@ impl IndexCoordinator { let shared = Arc::new(Mutex::new(Shared { mode: initial_mode, last_full_index: None, + files: None, queued_events: 0, watcher: WatcherStatus::Off, reconcile: None, @@ -217,6 +254,8 @@ impl IndexCoordinator { reconcile_done: None, reconcile_cut_short: false, saw_running: false, + files_at: None, + was_busy: false, write_conn: None, ignore: Arc::new(IgnoreSet::compile(&[]).expect("empty ignore set")), registry: Registry::default_set(), @@ -246,6 +285,7 @@ impl IndexCoordinator { mode: shared.mode, activity: self.indexing.get_status(), last_full_index: shared.last_full_index, + files: shared.files, queued_events: shared.queued_events, watcher: shared.watcher.clone(), reconcile: shared.reconcile, @@ -431,6 +471,17 @@ struct Inner { /// A start was requested; set false once the service reports running, /// so idle-after-running transitions are detectable. saw_running: bool, + /// When the published file count was last read, so it can be refreshed on + /// an interval rather than every tick. `None` forces the next tick to + /// re-read it. + files_at: Option, + /// Something has happened since the last time this coordinator settled. + /// + /// Drives [`Inner::go_idle`], which must fire once per busy→idle + /// transition rather than once per tick: releasing the page cache and + /// trimming the heap are both worth doing when the work stops and both are + /// pure overhead every second thereafter. + was_busy: bool, write_conn: Option, /// Shared with the watcher, which filters registrations by the same set. ignore: Arc, @@ -484,8 +535,10 @@ impl Inner { if let Err(e) = self.reload_filters() { crate::log_warn!("coordinator: {}", e); } - // The write connection may point at an old database_path. + // The write connection may point at an old database_path, and + // so may the count read through it. self.write_conn = None; + self.files_at = None; // A wipe stays the caller's decision — it is destructive and // the GUI may have to ask first (see `rebuild_index`). // Everything short of one this thread reconciles itself, in @@ -510,6 +563,11 @@ impl Inner { CoordCmd::RebuildIndex => { let db = self.db_path(); self.write_conn = None; + // The file is about to be deleted, so the published count is + // about to be wrong by all of it. Re-read on the next tick + // rather than letting the interval carry a stale figure across + // a wipe the user just asked for. + self.files_at = None; // Nothing to reconcile against once the file is gone — and // nothing to report about what was reconciled in the index // that is about to stop existing. @@ -534,7 +592,14 @@ impl Inner { if let Err(e) = self.indexing.delete_index_for_rebuild(&db) { crate::log_warn!("coordinator: clear index: {}", e); } - self.shared.lock().unwrap().last_full_index = None; + let mut shared = self.shared.lock().unwrap(); + shared.last_full_index = None; + // Zero, not `None`: nothing is going to rebuild this index, so + // there is no later read to correct a stale figure, and "0 + // files indexed" is the truth about what was just deleted. + shared.files = Some(0); + drop(shared); + self.files_at = None; } CoordCmd::Shutdown => unreachable!("handled in run()"), } @@ -564,12 +629,18 @@ impl Inner { // resolve the manual-run mode. if self.saw_running { self.saw_running = false; + self.was_busy = true; self.refresh_last_full_index(); + // Eagerly, not on the usual interval: the number the run just + // changed is the one the user is looking at when it finishes. + self.files_at = None; if self.mode == IndexMode::ManualRunning { self.mode = IndexMode::ManualStopped; } } + self.refresh_file_count(); + // Ahead of the mode gate: a config edit is reconciled in manual mode // too. It may end by starting a run, which is why this cannot wait for // the Auto-only scheduling below. @@ -582,16 +653,51 @@ impl Inner { if self.mode == IndexMode::ManualStopped { self.clear_pending(); } + self.go_idle(); return; } + let mut worked = false; if !self.pending.is_empty() && !self.needs_full_run && self.pending_settled() { self.apply_pending(); + worked = true; } if self.needs_full_run || self.periodic_due() { self.start_full_run(); + worked = true; } + + // Only when this tick found nothing to do. A tick that applied a batch + // is very likely to be followed by another that does the same, and + // releasing the connection between them would reopen it a moment later + // with a cold cache. + if !worked { + self.go_idle(); + } + } + + /// Settle: hand back what the work needed and the process no longer does. + /// + /// Two things, and they have to happen in this order. Dropping + /// [`Inner::write_conn`] closes a connection whose page cache a + /// reconciliation pass can have filled to its ceiling, and which is + /// otherwise held for the life of the process — + /// [`Inner::ensure_write_conn`] reopens it lazily, and cheaply, because + /// the key is applied in raw form and never re-derived. Then + /// [`crate::platform::release_free_heap`] returns those pages, and + /// everything the last indexing run and the search worker freed into + /// their arenas, to the kernel. + /// + /// Gated on [`Inner::was_busy`] so it runs once when the work stops rather + /// than every tick forever after. + fn go_idle(&mut self) { + if !self.was_busy { + return; + } + self.was_busy = false; + self.write_conn = None; + crate::platform::release_free_heap(); } fn drain_events(&mut self) { @@ -602,6 +708,7 @@ impl Inner { } if received { let now = Instant::now(); + self.was_busy = true; self.last_event_at = Some(now); self.pending_since.get_or_insert(now); // Before the overflow test, not after: an `rm -rf` of half a @@ -641,6 +748,7 @@ impl Inner { /// Advance the queued reconciliation by one slice, and start the full run /// it asked for once it is finished. fn apply_work(&mut self) { + self.was_busy = true; let mut conn = match self.ensure_write_conn() { Ok(conn) => conn, Err(e) => { @@ -744,6 +852,11 @@ impl Inner { /// `pending_since` cannot force an immediate apply of the next event. fn clear_pending(&mut self) { self.pending.clear(); + // `clear` empties the map but keeps the table it grew into, and this + // one grows to [`PENDING_OVERFLOW`] — an `rm -rf` of a large watched + // tree leaves a 100k-slot allocation behind for the life of the + // process. Releasing it is the point of clearing here at all. + self.pending.shrink_to_fit(); self.last_event_at = None; self.pending_since = None; } @@ -769,6 +882,7 @@ impl Inner { /// create-then-delete ends with it absent, because the upsert half consults /// the filesystem and finds nothing there. fn apply_pending(&mut self) { + self.was_busy = true; let mut conn = match self.ensure_write_conn() { Ok(conn) => conn, Err(e) => { @@ -846,7 +960,12 @@ impl Inner { if let Some(conn) = self.write_conn.take() { return Ok(conn); } - db::open_existing(&self.db_path(), true) + // Not `open_existing(_, true)`: that hands out the bulk indexer's + // profile, and this connection outlives every run. See + // [`db::schema::PRAGMAS_INCREMENTAL`]. Reopening is cheap — the key is + // applied in raw form and never re-derived — which is what makes + // dropping it in `go_idle` reasonable. + db::open::open_incremental_writer(&self.db_path()) } fn periodic_due(&self) -> bool { @@ -1084,6 +1203,43 @@ impl Inner { /// contended database as "never" would schedule a fresh run every tick /// for as long as the condition lasts. Keeping the previous value leaves /// the schedule where it was until a read succeeds. + /// Re-read the published row count, at most every [`FILE_COUNT_INTERVAL`]. + /// + /// Called only from the idle half of [`Inner::tick`], so it cannot run + /// while a full run holds the database. `COUNT(*)` is answered from the + /// narrowest index rather than the table (see [`db::repo::row_count`]), + /// but it is still a key scan of every row, so it runs behind an interval + /// and behind the interrupt guard that lets shutdown cut it short rather + /// than waiting out a scan of a several-million-row index. + fn refresh_file_count(&mut self) { + if let Some(at) = self.files_at { + if at.elapsed() < FILE_COUNT_INTERVAL { + return; + } + } + // Stamped before the read, not after: a count that keeps failing must + // back off exactly as a successful one does, or a missing index turns + // into an open attempt every tick. + self.files_at = Some(Instant::now()); + + let Ok(conn) = db::open_existing(&self.db_path(), false) else { + // No readable index yet. The status bar says "0 files indexed", + // which is both true and what the empty case should look like. + return; + }; + // The same slot `apply_work` arms: it is how the thread tearing this + // one down cuts short whatever statement the coordinator is inside, + // which a command cannot do because this thread is the one that would + // read the command. + let _guard = db::InterruptGuard::arm(&self.reconcile_stop.interrupt, &conn); + match db::repo::row_count(&conn) { + Ok(n) => self.shared.lock().unwrap().files = Some(n as i64), + // Interrupted by a shutdown, or a torn index a run will rebuild. + // Either way the last figure is better than none. + Err(e) => crate::log_warn!("coordinator: file count unavailable: {}", e), + } + } + fn refresh_last_full_index(&self) { match db::open_existing(&self.db_path(), false) { Ok(conn) => { diff --git a/crates/quicksearch-core/src/db/mod.rs b/crates/quicksearch-core/src/db/mod.rs index 33d6c40..bfcae8f 100644 --- a/crates/quicksearch-core/src/db/mod.rs +++ b/crates/quicksearch-core/src/db/mod.rs @@ -23,6 +23,36 @@ pub use open::{ CURRENT_SCHEMA_VERSION, KEY_MISMATCH_PREFIX, }; +/// Bumped whenever the index file is replaced rather than modified. +/// +/// Process-wide, like [`key::process_key`], and for the same reason: it is a +/// fact about the index this process is working with, not about any one +/// connection to it. +/// +/// Anything holding a connection open across operations needs to know that the +/// file underneath it has been swapped, and the path cannot tell it — a +/// rebuild and a clear both put a *new* file at the *same* path. A connection +/// that missed the change keeps serving the deleted inode: stale results, and +/// on Linux the old file's blocks stay allocated for as long as the handle +/// lives. [`crate::search`] is the only long-lived reader today; it compares +/// this against the value it opened with. +static INDEX_EPOCH: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + +/// The current index generation. See [`INDEX_EPOCH`]. +pub fn index_epoch() -> u64 { + INDEX_EPOCH.load(std::sync::atomic::Ordering::SeqCst) +} + +/// Declare that the index file has been replaced. +/// +/// Called from [`open::open_or_recreate`]'s wipe path — the one place a wipe +/// actually happens, so a schema drift nobody asked for cannot slip past — +/// and from the coordinator's explicit rebuild and clear commands, which +/// delete the file without going through it. +pub fn bump_index_epoch() { + INDEX_EPOCH.fetch_add(1, std::sync::atomic::Ordering::SeqCst); +} + /// A shared slot holding the interrupt handle of whatever long statement is /// running, so another thread can cut it short. /// diff --git a/crates/quicksearch-core/src/db/open.rs b/crates/quicksearch-core/src/db/open.rs index 81cff65..25e53c8 100644 --- a/crates/quicksearch-core/src/db/open.rs +++ b/crates/quicksearch-core/src/db/open.rs @@ -18,8 +18,8 @@ use std::path::Path; use rusqlite::{params, Connection, OpenFlags, OptionalExtension}; use super::schema::{ - effective_tokenizer, fts_create_sql, PRAGMAS_FAST, PRAGMAS_MAINTENANCE, PRAGMAS_READONLY, - PRAGMAS_WALK_READER, SCHEMA_CURRENT, + effective_tokenizer, fts_create_sql, PRAGMAS_FAST, PRAGMAS_INCREMENTAL, PRAGMAS_MAINTENANCE, + PRAGMAS_READONLY, PRAGMAS_SEARCH, PRAGMAS_WALK_READER, SCHEMA_CURRENT, }; use crate::security::IndexKey; @@ -119,6 +119,35 @@ pub fn open_walk_reader(db_path: &str) -> Result { ) } +/// The search worker's connection, held across requests. +/// +/// Identical to `open_existing(_, false)` except for the pragma profile: see +/// [`PRAGMAS_SEARCH`] for why this one is deliberately larger than the +/// one-shot readers' and still smaller than what it replaced. +pub fn open_search_reader(db_path: &str) -> Result { + open_keyed_with_pragmas( + db_path, + false, + super::key::process_key().as_ref(), + PRAGMAS_SEARCH, + ) +} + +/// The coordinator's write connection, for applying watcher events and +/// reconciling a config change. +/// +/// Identical to `open_existing(_, true)` except for the pragma profile: see +/// [`PRAGMAS_INCREMENTAL`] for why the process's longest-lived writer must not +/// take the cache the bulk indexer does. +pub fn open_incremental_writer(db_path: &str) -> Result { + open_keyed_with_pragmas( + db_path, + true, + super::key::process_key().as_ref(), + PRAGMAS_INCREMENTAL, + ) +} + /// A writable connection for post-run compaction, and the only one that may /// VACUUM. See [`PRAGMAS_MAINTENANCE`] for why it cannot be the indexer's. pub fn open_maintenance(db_path: &str) -> Result { @@ -369,6 +398,10 @@ fn wipe_and_reopen( key: Option<&IndexKey>, ) -> Result { drop(conn); + // Before the delete, not after: anything holding a connection to the file + // about to disappear must be told, and told even if the removal below + // fails partway. See [`super::bump_index_epoch`]. + super::bump_index_epoch(); // Primary file may already be absent (fresh open that just needed // the table applied). Ignore NotFound; anything else is an error. // diff --git a/crates/quicksearch-core/src/db/schema.rs b/crates/quicksearch-core/src/db/schema.rs index 0b5d41a..bd91af5 100644 --- a/crates/quicksearch-core/src/db/schema.rs +++ b/crates/quicksearch-core/src/db/schema.rs @@ -3,8 +3,63 @@ //! Versioned by [`super::open::CURRENT_SCHEMA_VERSION`], but there is no //! upgrade path: a database written under any other version is wiped and //! recreated from [`SCHEMA_CURRENT`]. See [`super::open`] for why. +//! +//! # The pragma profiles, and why there are six of them +//! +//! Each profile below belongs to one kind of connection, and the only field +//! that differs between most of them is `cache_size`. They are separate +//! constants rather than one shared string because the page cache is the +//! largest thing a connection holds, and what the right size is depends +//! entirely on what the connection does and how long it lives — a question +//! with six different answers here. +//! +//! **A negative `cache_size` is KiB; a positive one would be a page count.** +//! Nothing sets `page_size`, so a page count would be an unfalsifiable claim +//! about SQLCipher's default, and SQLCipher reserves per-page bytes for the IV +//! and HMAC on top of that, so pages do not convert to bytes by a clean +//! multiply. Every value here is a ceiling in KiB, which is the thing actually +//! being reasoned about. +//! +//! It *is* a ceiling and not a reservation — a connection that touches ten +//! pages holds ten pages. What matters is which connections can reach the +//! ceiling, which is any of them that scans a table, and how long they hold it +//! afterwards. Page cache is `malloc`ed in 4 KiB units, far below glibc's mmap +//! threshold, so a filled cache is arena memory: closing the connection +//! returns it to the arena, not to the kernel. That is why these numbers show +//! up in an *idle* process's footprint at all, and why +//! [`crate::platform::release_free_heap`] exists alongside them. +//! +//! Note which way each profile is sized. Every one of them is small because +//! its connection either scans once or lives a long time — except +//! [`PRAGMAS_SEARCH`], which is large because it is the only cache that is +//! *reused* often enough to pay for itself, and which is released when +//! searching stops so that it never becomes part of the idle floor. +//! +//! | Profile | Connection | Lifetime | Cache | +//! |---|---|---|---| +//! | [`PRAGMAS_FAST`] | bulk indexer writer | one run | 8 MiB | +//! | [`PRAGMAS_INCREMENTAL`] | coordinator's writer | released when idle | 4 MiB | +//! | [`PRAGMAS_SEARCH`] | search worker | held across a typing session | 32 MiB | +//! | [`PRAGMAS_READONLY`] | one-shot readers | a single query | 4 MiB | +//! | [`PRAGMAS_MAINTENANCE`] | VACUUM | one bulk copy | 8 MiB | +//! | [`PRAGMAS_WALK_READER`] | per-root row prefetch | the walk | 1 MiB | +//! +//! `PRAGMA mmap_size` is deliberately absent from all of them. It is not +//! compile-disabled, and SQLCipher's codec only turns it off at runtime when a +//! key is set — so an unprotected index could use it. It stays off because +//! mapped pages still count in `VmRSS`, so it would not help the number this +//! is all about, and because it would make memory behaviour differ between +//! protected and unprotected installs, which is exactly the kind of silent +//! per-configuration difference [`crate::platform`] argues against. -/// Pragmas applied on every writable connection open. +/// The bulk indexer's write connection: one per run, dies with it. +/// +/// 8 MiB of page cache, not the 40 MiB this profile used to take. The cache +/// buys a writer the chance to batch dirty pages before spilling mid +/// transaction, and a run commits every `batch_size` rows (200–500), so the +/// working set between commits is nowhere near 40 MiB. The old figure was +/// never chosen for this connection — it was chosen once and then inherited by +/// every other profile that copied this one. /// /// WAL, not journal-off: auto-indexing writes continuously while searches /// stream from their own read-only connections, and WAL is what lets those @@ -21,7 +76,7 @@ pub const PRAGMAS_FAST: &str = " PRAGMA journal_mode = WAL; PRAGMA synchronous = NORMAL; PRAGMA busy_timeout = 5000; - PRAGMA cache_size = 10000; + PRAGMA cache_size = -8192; PRAGMA temp_store = MEMORY; PRAGMA foreign_keys = ON; "; @@ -36,24 +91,98 @@ pub const PRAGMAS_FAST: &str = " /// entire rebuilt index in RAM. See [`super::repo::maintain`], which also /// points the temp directory at the index's own volume. /// -/// The smaller page cache is because this connection does one bulk copy and -/// then closes; the 40 MiB the indexer keeps hot buys it nothing. +/// The page cache is sized for one bulk copy followed by a close, which is +/// all this connection ever does. pub const PRAGMAS_MAINTENANCE: &str = " PRAGMA journal_mode = WAL; PRAGMA synchronous = NORMAL; PRAGMA busy_timeout = 5000; - PRAGMA cache_size = 2000; + PRAGMA cache_size = -8192; PRAGMA temp_store = FILE; PRAGMA foreign_keys = ON; "; +/// The coordinator's long-lived write connection. +/// +/// [`PRAGMAS_FAST`] without the `journal_mode`/`synchronous` lines it does not +/// need to set again, and with a smaller cache, which is why it is its own +/// profile. This connection exists to apply a handful of single-row upserts +/// and deletes per batch of watcher events; it has no bulk phase to batch for. +/// +/// The size matters more here than anywhere else because of the lifetime. This +/// is the one connection that lives as long as the process, so whatever it +/// reaches, it holds — and [`super::super::scope::advance`] runs a +/// forward-only scan of `files` through it after a config change, which is +/// exactly the access pattern that fills a cache to its ceiling. Before this +/// profile existed that meant an idle QuickSearch carried a full 40 MiB page +/// cache from a reconciliation the user did once. It is now also dropped +/// outright when the coordinator settles (see `Inner::go_idle`); this profile +/// bounds what it can reach *before* then. +pub const PRAGMAS_INCREMENTAL: &str = " + PRAGMA journal_mode = WAL; + PRAGMA synchronous = NORMAL; + PRAGMA busy_timeout = 5000; + PRAGMA cache_size = -4096; + PRAGMA temp_store = MEMORY; + PRAGMA foreign_keys = ON; +"; + +/// The search worker's connection, which is held across requests. +/// +/// A search fires on every character typed, and the worker keeps one +/// connection for the whole typing session rather than opening one per +/// keystroke (see [`crate::search`]). That inverts what the cache is for: it +/// is not there to absorb a single cold cascade, it is there to still be warm +/// when the next character arrives. +/// +/// **This is the one profile that is deliberately large, and the size is +/// measured rather than reasoned.** `tests/search_perf.rs` sweeps it; on an +/// encrypted index the curve is not a gradient but a cliff, and the cliff is +/// at the working set: +/// +/// | ceiling | warm, unencrypted | warm, encrypted | +/// |---|---|---| +/// | 32–40 MiB | ~19 ms | **~19 ms** | +/// | 1–16 MiB | ~20 ms | **~47 ms** | +/// +/// Unencrypted, the ceiling makes no difference at all — a miss is a `pread` +/// from the OS page cache and a `memcpy`. Encrypted, SQLCipher caches pages +/// *decrypted*, so a hit skips an AES-CBC decrypt and an HMAC-SHA512 verify +/// per 4 KiB; below the working set every warm query pays for all of them +/// again, which is the 2.5× above. One number covers both because sizing this +/// by whether a key happens to be set would make search latency depend on a +/// setting nobody would connect it to. +/// +/// The ceiling only stands while someone is searching: the worker releases the +/// connection after [`crate::search`]'s idle window, and +/// [`crate::platform::release_free_heap`] returns the pages. So this buys warm +/// search latency without adding to what an idle process holds — which is the +/// trade the rest of these profiles are making in the other direction. +/// +/// The knee tracks index size, so on a very large encrypted index even this +/// will not hold the working set. That degrades to the old behaviour rather +/// than to something worse, and the fix if it ever matters is a bigger number +/// here, informed by the same test. +pub const PRAGMAS_SEARCH: &str = " + PRAGMA busy_timeout = 5000; + PRAGMA cache_size = -32768; + PRAGMA temp_store = MEMORY; + PRAGMA foreign_keys = ON; +"; + /// Pragmas safe to apply on a read-only connection, where `journal_mode` /// and `synchronous` can't be changed on the file. Used by /// [`super::open::open_existing`] for read-only opens; write paths get the /// full [`PRAGMAS_FAST`] set. +/// +/// This is the *one-shot* reader now that the search worker has +/// [`PRAGMAS_SEARCH`]: the CLI query helpers, the duplicates scan, the +/// coordinator's own small reads. Each opens, runs a single query, and closes. +/// A cache only pays for itself across queries, and these connections have no +/// across. pub const PRAGMAS_READONLY: &str = " PRAGMA busy_timeout = 5000; - PRAGMA cache_size = 10000; + PRAGMA cache_size = -4096; PRAGMA temp_store = MEMORY; PRAGMA foreign_keys = ON; "; @@ -62,17 +191,17 @@ pub const PRAGMAS_READONLY: &str = " /// /// Identical to [`PRAGMAS_READONLY`] but for `cache_size`, and that one /// difference is the point. One of these connections exists per indexing -/// root, so the 10000-page (~40 MiB) cache the other profiles take would -/// cost ~200 MiB across five roots — more than the per-directory -/// classification this connection exists to serve was meant to save. +/// root, so a cache sized for a connection that runs alone would be +/// multiplied by the root count — and this was the first profile to be sized +/// for its access pattern rather than copied from [`PRAGMAS_FAST`], which is +/// why the others now are too. /// -/// 256 pages (~1 MiB) is enough to hold the upper levels of -/// `idx_files_parent` hot, which is all these queries touch: each one is a -/// single index range lookup, and the pages under it are read once and not -/// revisited. +/// 1 MiB is enough to hold the upper levels of `idx_files_parent` hot, which +/// is all these queries touch: each one is a single index range lookup, and +/// the pages under it are read once and not revisited. pub const PRAGMAS_WALK_READER: &str = " PRAGMA busy_timeout = 5000; - PRAGMA cache_size = 256; + PRAGMA cache_size = -1024; PRAGMA temp_store = MEMORY; PRAGMA foreign_keys = ON; "; diff --git a/crates/quicksearch-core/src/extract/pdf.rs b/crates/quicksearch-core/src/extract/pdf.rs index e6da8e5..c630947 100644 --- a/crates/quicksearch-core/src/extract/pdf.rs +++ b/crates/quicksearch-core/src/extract/pdf.rs @@ -1,16 +1,30 @@ //! PDF text extraction. //! -//! Primary path: [`pdf_extract::extract_text`], which handles most modern PDFs -//! and is simple to call. It can panic or hard-error on malformed files; any -//! failure is surfaced to the caller and marks the file's content state as -//! failed. Properties (title, author, etc.) from the PDF `Info` dictionary -//! are pulled via `lopdf` where available. +//! One `Document::load` per file, then both the text and the `Info` dictionary +//! are taken off it. `pdf_extract` can panic or hard-error on malformed files; +//! any failure is surfaced to the caller and marks the file's content state as +//! failed. +//! +//! This used to be two loads of the same file — `pdf_extract::extract_text`, +//! which parses the document internally, followed by a second parse through a +//! directly-declared `lopdf` purely to read six `Info` strings. Both object +//! graphs and the full extracted text were live at once, on the extractor that +//! `examples/memprobe.rs` already named as a run's largest single consumer. +//! Naming `lopdf` as a direct dependency also resolved a *second, older* copy +//! of it, which is what dragged `rayon` — and a global thread pool that is +//! never torn down — plus `chrono`, `time`, `md5` and a second `nom` into the +//! build. +//! +//! So `lopdf` is reached through `pdf_extract`'s own `pub use lopdf::*` and +//! must not be declared in `Cargo.toml` again. The re-export is not a +//! semver-guaranteed surface, but a break in it is a compile error rather than +//! a silent behaviour change, and the fixtures below cover the behaviour. use std::cell::Cell; use std::path::Path; use std::sync::OnceLock; -use lopdf::{Document as LopdfDocument, Object}; +use pdf_extract::{Document, Object, PlainTextOutput}; use super::{ExtractError, ExtractedContent, Extractor}; @@ -56,45 +70,69 @@ impl Extractor for PdfExtractor { } fn extract(&self, path: &Path) -> Result { - // Text. Catch panics from pdf_extract (some PDFs crash its parser) - // and keep the default hook from spamming stderr about them. + // Catch panics from pdf_extract (some PDFs crash its parser) and keep + // the default hook from spamming stderr about them. The whole + // operation is inside the guard, document loading included — that used + // to sit outside both it and the suppression window, so a panic in the + // parser reached the process hook and took the thread with it. install_quiet_panic_hook(); let path_buf = path.to_path_buf(); SUPPRESS_PANIC_PRINT.with(|flag| flag.set(true)); - let result = std::panic::catch_unwind(move || pdf_extract::extract_text(&path_buf)); + let result = std::panic::catch_unwind(move || extract_one_pass(&path_buf)); SUPPRESS_PANIC_PRINT.with(|flag| flag.set(false)); - let text = result - .map_err(|panic| format!("pdf_extract panicked: {}", panic_message(&*panic)))? - .map_err(|e| format!("pdf_extract: {}", e))?; + result.map_err(|panic| format!("pdf_extract panicked: {}", panic_message(&*panic)))? + } +} - let mut out = ExtractedContent::with_text(text); +/// The six `Info` keys worth keeping, in the order they are written. +const INFO_KEYS: [&str; 6] = [ + "Title", "Author", "Subject", "Keywords", "Creator", "Producer", +]; - // Info dictionary via lopdf. Soft-fail: if lopdf can't open the file - // we still return the text. - if let Ok(doc) = LopdfDocument::load(path) { - if let Ok(info_ref) = doc.trailer.get(b"Info") { - if let Ok(info_id) = info_ref.as_reference() { - if let Ok(info) = doc.get_object(info_id) { - if let Ok(dict) = info.as_dict() { - for key in [ - "Title", "Author", "Subject", "Keywords", "Creator", "Producer", - ] { - if let Ok(val) = dict.get(key.as_bytes()) { - if let Some(s) = object_to_string(val) { - if !s.is_empty() { - out.properties.insert(key.to_ascii_lowercase(), s); - } - } - } - } - } - } +/// Load the document once; take the text and the `Info` dictionary off it. +/// +/// This is `pdf_extract::extract_text` — whose body is load, decrypt, +/// `output_doc` — with the `Info` read folded in where the document is still +/// in scope, which is the entire reason it is spelled out here rather than +/// called. +fn extract_one_pass(path: &Path) -> Result { + let mut doc = Document::load(path).map_err(|e| format!("pdf_extract: {}", e))?; + // What `pdf_extract`'s own (private) `maybe_decrypt` does, and it has to + // happen before either the content streams or the `Info` strings mean + // anything — the previous two-load version never decrypted for the `Info` + // half, so those properties were garbage on any encrypted file. Empty + // password only: a real one is the user's to supply and nothing on this + // path can ask for it. + if doc.is_encrypted() { + doc.decrypt("").map_err(|e| format!("pdf_extract: {}", e))?; + } + + let mut text = String::new(); + { + let mut sink = PlainTextOutput::new(&mut text); + pdf_extract::output_doc(&doc, &mut sink).map_err(|e| format!("pdf_extract: {}", e))?; + } + let mut out = ExtractedContent::with_text(text); + + // Soft-fail, unchanged: a document with no readable `Info` dictionary + // still has its text, and the text is the half that matters. + let info = doc + .trailer + .get(b"Info") + .ok() + .and_then(|o| o.as_reference().ok()) + .and_then(|id| doc.get_object(id).ok()) + .and_then(|o| o.as_dict().ok()); + if let Some(dict) = info { + for key in INFO_KEYS { + if let Some(s) = dict.get(key.as_bytes()).ok().and_then(object_to_string) { + if !s.is_empty() { + out.properties.insert(key.to_ascii_lowercase(), s); } } } - - Ok(out) } + Ok(out) } fn object_to_string(obj: &Object) -> Option { @@ -131,4 +169,173 @@ mod tests { assert!(PdfExtractor.supports("application/pdf")); assert!(!PdfExtractor.supports("application/zip")); } + + use pdf_extract::{dictionary, Dictionary, Stream, StringFormat}; + use std::path::PathBuf; + + /// Write a one-page PDF drawing `body`, with `info` as its `Info` + /// dictionary, and return the path. + /// + /// Built here rather than checked in as a blob, following the convention + /// [`super::super::ole`] states: a fixture asserted to have a particular + /// structure is not reviewable when it is opaque bytes. Everything needed + /// is public through `pdf_extract`'s `lopdf` re-export, which is the same + /// surface the extractor itself uses — so if that re-export ever moves, + /// these fail to compile alongside it rather than silently stopping + /// covering anything. + /// + /// The page is deliberately minimal but complete: `output_doc` walks + /// Catalog → Pages → Page and needs `MediaBox`, a `Resources` font it can + /// resolve, and a content stream. Helvetica is one of the base-14 fonts, + /// for which `pdf_extract` carries built-in encoding tables, so no font + /// file is involved. + fn write_pdf(tag: &str, body: &str, info: Option) -> PathBuf { + let mut doc = Document::with_version("1.5"); + let font = doc.add_object(dictionary! { + "Type" => "Font", + "Subtype" => "Type1", + "BaseFont" => "Helvetica", + }); + let resources = doc.add_object(dictionary! { + "Font" => dictionary! { "F1" => font }, + }); + let content = format!("BT /F1 24 Tf 72 720 Td ({}) Tj ET", body); + let contents = doc.add_object(Stream::new(dictionary! {}, content.into_bytes())); + let pages_id = doc.new_object_id(); + let page = doc.add_object(dictionary! { + "Type" => "Page", + "Parent" => pages_id, + "Contents" => contents, + "MediaBox" => vec![0.into(), 0.into(), 612.into(), 792.into()], + }); + doc.objects.insert( + pages_id, + Object::Dictionary(dictionary! { + "Type" => "Pages", + "Kids" => vec![page.into()], + "Count" => 1, + "Resources" => resources, + }), + ); + let catalog = doc.add_object(dictionary! { + "Type" => "Catalog", + "Pages" => pages_id, + }); + doc.trailer.set("Root", catalog); + if let Some(info) = info { + let info_id = doc.add_object(Object::Dictionary(info)); + doc.trailer.set("Info", info_id); + } + + let path = crate::testutil::scratch_dir(tag).join("fixture.pdf"); + doc.save(&path).expect("write fixture pdf"); + path + } + + fn text_string(s: &str) -> Object { + Object::String(s.as_bytes().to_vec(), StringFormat::Literal) + } + + #[test] + fn extracts_text_and_info_properties() { + let path = write_pdf( + "pdf-full", + "Hello QuickSearch", + Some(dictionary! { + "Title" => text_string("The Title"), + "Author" => text_string("An Author"), + "Subject" => text_string("A Subject"), + "Keywords" => text_string("alpha beta"), + "Creator" => text_string("A Creator"), + "Producer" => text_string("A Producer"), + }), + ); + + let out = PdfExtractor.extract(&path).expect("extract"); + assert!( + out.text.contains("Hello QuickSearch"), + "drawn text missing from {:?}", + out.text + ); + // Lowercased keys, which is the contract the rest of the pipeline + // stores under. + assert_eq!(out.properties.get("title").map(String::as_str), Some("The Title")); + assert_eq!(out.properties.get("author").map(String::as_str), Some("An Author")); + assert_eq!(out.properties.get("subject").map(String::as_str), Some("A Subject")); + assert_eq!(out.properties.get("keywords").map(String::as_str), Some("alpha beta")); + assert_eq!(out.properties.get("creator").map(String::as_str), Some("A Creator")); + assert_eq!(out.properties.get("producer").map(String::as_str), Some("A Producer")); + } + + /// The soft-fail path: no `Info` dictionary is not an extraction failure, + /// because the text is the half that matters. + #[test] + fn missing_info_dictionary_still_yields_text() { + let path = write_pdf("pdf-noinfo", "Body Only", None); + let out = PdfExtractor.extract(&path).expect("extract"); + assert!(out.text.contains("Body Only")); + assert!( + out.properties.is_empty(), + "unexpected properties: {:?}", + out.properties + ); + } + + /// An empty `Info` value is absence, not an empty property. + #[test] + fn empty_info_values_are_not_stored() { + let path = write_pdf( + "pdf-emptyinfo", + "Body", + Some(dictionary! { + "Title" => text_string(""), + "Author" => text_string("Real Author"), + }), + ); + let out = PdfExtractor.extract(&path).expect("extract"); + assert!(!out.properties.contains_key("title"), "empty title stored"); + assert_eq!( + out.properties.get("author").map(String::as_str), + Some("Real Author") + ); + } + + /// `Info` values that are not strings or names are skipped rather than + /// rendered — pins `object_to_string`'s catch-all arm. + #[test] + fn non_string_info_values_are_skipped() { + let path = write_pdf( + "pdf-badinfo", + "Body", + Some(dictionary! { + "Producer" => 42, + "Title" => text_string("Kept"), + }), + ); + let out = PdfExtractor.extract(&path).expect("extract"); + assert!( + !out.properties.contains_key("producer"), + "integer Info value was rendered: {:?}", + out.properties + ); + assert_eq!(out.properties.get("title").map(String::as_str), Some("Kept")); + } + + /// Malformed input must come back as an error, not take the process down. + /// This is the case the widened `catch_unwind` exists for: the document + /// load now runs inside it, where it used to run outside. + #[test] + fn malformed_pdf_fails_without_panicking_the_process() { + let path = crate::testutil::scratch_dir("pdf-malformed").join("broken.pdf"); + std::fs::write(&path, b"%PDF-1.4\n\x00\x01\x02 not a pdf at all \xff\xfe").unwrap(); + + let err = PdfExtractor + .extract(&path) + .expect_err("malformed pdf must fail"); + assert!( + err.starts_with("pdf_extract"), + "unexpected failure reason: {}", + err + ); + } } diff --git a/crates/quicksearch-core/src/indexing.rs b/crates/quicksearch-core/src/indexing.rs index b57eb68..e87d9ab 100644 --- a/crates/quicksearch-core/src/indexing.rs +++ b/crates/quicksearch-core/src/indexing.rs @@ -713,6 +713,12 @@ impl IndexingService { } } + // Before the removal, and unconditionally: readers holding this file + // open have to be told even if the delete below fails, because the + // half-deleted case is exactly the one where a stale handle does + // damage. See [`db::bump_index_epoch`]. + db::bump_index_epoch(); + if std::path::Path::new(db_path).exists() { std::fs::remove_file(db_path) .map_err(|e| format!("Failed to delete database file: {}", e))?; @@ -796,6 +802,17 @@ impl IndexingService { *status_clone.lock().unwrap() = IndexingStatus::Idle; } } + + // A run's whole working set becomes garbage at once + // here: the walk's in-flight buffers, every extractor's + // scratch, `seen_paths`, and the writer's page cache. + // Under glibc none of it goes back to the OS on its + // own, so a peak measured in hundreds of megabytes + // would otherwise be the process's floor for as long as + // it stays open. After the match, not inside it, + // because a failed or stopped run leaves exactly as + // much behind as a successful one. + crate::platform::release_free_heap(); })); } IndexingCommand::Stop => { diff --git a/crates/quicksearch-core/src/platform.rs b/crates/quicksearch-core/src/platform.rs index ae9fcc5..167cf99 100644 --- a/crates/quicksearch-core/src/platform.rs +++ b/crates/quicksearch-core/src/platform.rs @@ -9,6 +9,12 @@ //! - Anything that can be decided from a string rather than a syscall is //! split out and made testable everywhere ([`is_unc_string`], //! [`PATH_COLLATION`]), because the test suite runs on Linux. +//! +//! Beyond filesystem and path semantics, this is also where the process's +//! dealings with its own allocator and threads live — [`release_free_heap`] +//! and [`heap_stats`], which are glibc-only, and [`spawn_worker`], which puts +//! the run's worker stack size in one place. They are here for the same reason +//! everything else is: they are `#[cfg]`, and `#[cfg]` lives here. use std::ffi::OsString; use std::path::{Component, Path, PathBuf}; @@ -534,6 +540,106 @@ pub fn set_background_priority() { // adding on its own terms rather than approximating here. } +/// Stack size for the per-run worker threads spawned by [`spawn_worker`]. +/// +/// Rust's default is 8 MiB of *reserved* address space, of which only touched +/// pages become resident — so on its own the default costs nothing much. What +/// costs is that glibc **caches freed thread stacks** rather than unmapping +/// them (`stack_cache_maxsize`, 40 MiB by default) and a cached stack keeps +/// its dirty pages. A run spawns walk and extraction workers per root, so the +/// pages those threads touched outlive them and sit in that cache for the life +/// of the process. +/// +/// 512 KiB is ample for what these threads actually do: bounded loops over a +/// directory's entries and over a batch of rows. The one thing here that +/// recurses on untrusted input is document parsing — OLE2 compound files and +/// zip/XML containers — and that runs on extraction workers, so if a malformed +/// document ever overflows this it should be raised rather than reverted, and +/// the parser given a depth limit. +const WORKER_STACK_SIZE: usize = 512 * 1024; + +/// Spawn one of a run's short-lived worker threads. +/// +/// Exists to put [`WORKER_STACK_SIZE`] in one place rather than at each of the +/// four spawn sites, and to name the threads while it is at it — `qs-walk`, +/// `qs-extract` and friends show up in `top -H` and in a debugger, which the +/// anonymous `thread::spawn` versions did not. +/// +/// Panics if the thread cannot be spawned, exactly as `thread::spawn` does: a +/// run that cannot start its workers has nothing to fall back to. +pub fn spawn_worker(name: &str, f: F) -> std::thread::JoinHandle +where + F: FnOnce() -> T + Send + 'static, + T: Send + 'static, +{ + std::thread::Builder::new() + .name(name.to_string()) + .stack_size(WORKER_STACK_SIZE) + .spawn(f) + .expect("spawn worker thread") +} + +/// Return free heap pages to the kernel. +/// +/// glibc's `free` returns a chunk to its arena's free list, not to the OS — +/// only the top of an arena is ever trimmed, and only past `M_TRIM_THRESHOLD`. +/// So a transient peak stays in RSS for the life of the process even though +/// nothing is using it. Indexing peaks around 200 MiB above baseline on a +/// large root (see `examples/memprobe.rs`), and a full-table scan fills a +/// connection's SQLite page cache with 4 KiB allocations that are individually +/// far below the mmap threshold — both land in an arena and stay there. This +/// is what gives them back. +/// +/// Process-wide despite being one call: `malloc_trim(0)` walks *every* arena, +/// so calling it on the coordinator's thread also reclaims what the search +/// worker and the finished indexing threads left behind. That is why there are +/// only two call sites rather than one per subsystem — and why it must not go +/// anywhere hot, since the walk plus the `madvise` per free page costs +/// milliseconds on a large heap. +/// +/// Best-effort and idempotent, like [`set_background_priority`]: a refusal +/// means only that the memory stays where it was. +pub fn release_free_heap() { + #[cfg(all(target_os = "linux", target_env = "gnu"))] + { + // SAFETY: no arguments, no pointers, and safe to call from any thread + // at any time — glibc takes the arena locks itself. + unsafe { libc::malloc_trim(0) }; + } + // Elsewhere: deliberately nothing. `malloc_trim` is a glibc extension — + // musl has no equivalent and does not need one (its allocator returns + // spans to the kernel on free), and the Windows CRT heap has + // `_heapmin`, which is worth adding on its own terms if Windows RSS ever + // proves to be a problem rather than assuming it behaves like glibc. +} + +/// Live and free-but-retained heap bytes, as `(in_use, free)`. +/// +/// The gap between the two *is* the retention this module's +/// [`release_free_heap`] exists to close: `in_use` is memory something still +/// holds, `free` is memory the program has already given back to the allocator +/// and which glibc is nonetheless still charging the process for. A large +/// `free` immediately after a trim is the signal that `malloc_trim` cannot +/// reach the fragmentation and a different allocator is the answer. +/// +/// `None` where the platform has no way to answer. +pub fn heap_stats() -> Option<(u64, u64)> { + #[cfg(all(target_os = "linux", target_env = "gnu"))] + { + // `mallinfo2`, not `mallinfo`: the older struct is `int`-typed and + // silently wraps past 2 GiB, which is exactly the size where the + // answer starts to matter. + // + // SAFETY: no arguments, returns a plain struct by value. + let info = unsafe { libc::mallinfo2() }; + Some((info.uordblks as u64, info.fordblks as u64)) + } + #[cfg(not(all(target_os = "linux", target_env = "gnu")))] + { + None + } +} + /// How long to keep retrying a delete that fails because something else holds /// the file open. #[cfg(windows)] diff --git a/crates/quicksearch-core/src/search/mod.rs b/crates/quicksearch-core/src/search/mod.rs index 17afe9f..4cb47ce 100644 --- a/crates/quicksearch-core/src/search/mod.rs +++ b/crates/quicksearch-core/src/search/mod.rs @@ -24,15 +24,35 @@ //! //! Consumers that want a plain blocking search (the CLI mode) skip the //! service entirely and call [`cascade::run`] with a collecting sink. +//! +//! # The worker's connection +//! +//! The worker holds one connection across requests and releases it after +//! [`IDLE_RELEASE`] of quiet. This is not an optimisation of open cost — an +//! open is microseconds — but of the page cache behind it: a search runs on +//! every character typed, so reopening per request meant paying to warm a +//! cache and then discarding it, once per keystroke, forever. +//! +//! What the old per-request open bought, and what now has to be arranged +//! deliberately, is never operating on an index that has been replaced +//! underneath it. Two things can do that, and only one is visible in the path: +//! the config can point at a different file, and a rebuild, clear or +//! schema-drift wipe can put a *new* file at the *same* path. The second is +//! why [`crate::db::index_epoch`] exists — see [`Worker::take_connection`]. +//! Holding a handle on a replaced index would mean stale results and, on +//! Linux, its blocks staying allocated until the handle closed. pub mod cascade; pub mod duplicates; pub mod fuzzy; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{mpsc, Arc, Mutex}; use std::thread::JoinHandle; +use std::time::Duration; + +use rusqlite::Connection; use crate::db; use crate::query::split::split_for_cascade; @@ -41,6 +61,19 @@ use crate::snippet::Snippet; pub use cascade::Outcome; pub use duplicates::{find_duplicate_groups, DuplicateGroup}; +/// How long the worker keeps its connection after the last request. +/// +/// The connection is held across requests so a typing session runs against a +/// warm page cache (see [`Worker::take_connection`]). It is not held *forever*, for +/// two reasons that have nothing to do with the cache: an open handle on a +/// deleted index keeps its blocks allocated on disk, and an open reader stops +/// SQLite from resetting the WAL, which would then grow toward +/// `maximum_wal_size` and never come back down. +/// +/// So: long enough to span the pauses inside a search session, short enough +/// that an abandoned one is not still holding the index minutes later. +const IDLE_RELEASE: Duration = Duration::from_secs(30); + /// One search result. `rank` is the sort key (lower = better): integer /// part = cascade stage (1–11), fraction = occurrence-count or /// edit-distance tiebreak. Batches arrive already rank-ordered and later @@ -147,6 +180,18 @@ impl SearchService { pub fn new( db_path: PathBuf, notify: Arc, + ) -> (SearchService, mpsc::Receiver) { + Self::new_with_idle_release(db_path, notify, IDLE_RELEASE) + } + + /// [`Self::new`] with an explicit connection-release window. + /// + /// Tests use short windows; the default [`IDLE_RELEASE`] is right for real + /// use, where the window has to span the pauses inside a typing session. + pub fn new_with_idle_release( + db_path: PathBuf, + notify: Arc, + idle_release: Duration, ) -> (SearchService, mpsc::Receiver) { let (req_tx, req_rx) = mpsc::channel::(); let (update_tx, update_rx) = mpsc::channel::(); @@ -161,6 +206,8 @@ impl SearchService { latest_gen: latest_gen.clone(), in_flight: in_flight.clone(), db_path: db_path.clone(), + open: None, + idle_release, }; let handle = std::thread::Builder::new() .name("qs-search".into()) @@ -268,11 +315,37 @@ struct Worker { latest_gen: Arc, in_flight: InFlight, db_path: Arc>, + /// The connection, and the index generation and path it was opened + /// against. See [`Worker::take_connection`]. + open: Option, + /// How long `open` survives with no requests; [`IDLE_RELEASE`] outside + /// tests. + idle_release: Duration, +} + +/// A connection held across requests, tagged with what it was opened on. +struct OpenIndex { + conn: Connection, + epoch: u64, + path: PathBuf, } impl Worker { - fn run(self) { - while let Ok(first) = self.req_rx.recv() { + fn run(mut self) { + loop { + // `recv_timeout`, not `recv`: a search session is a burst of + // requests one keystroke apart, and the connection is worth + // keeping for the length of one. Past that it is worth strictly + // less than nothing — see [`IDLE_RELEASE`]. + let first = match self.req_rx.recv_timeout(self.idle_release) { + Ok(req) => req, + Err(mpsc::RecvTimeoutError::Timeout) => { + self.open = None; + continue; + } + // The service was dropped; nothing more is coming. + Err(mpsc::RecvTimeoutError::Disconnected) => return, + }; // A fast typist queues several requests; only the newest one // matters. let mut req = first; @@ -286,12 +359,50 @@ impl Worker { } } + /// Take the connection to run this request on, reopening if the one held + /// cannot be reused. + /// + /// Taken out and handed back by the caller, rather than borrowed from + /// `self`, for the same reason [`crate::coordinator`]'s writer is: the + /// cascade needs the connection for its whole run, and everything else on + /// `self` — the update channel, the generation counter, the interrupt slot + /// — has to stay reachable while it does. + /// + /// Reuse is what makes [`crate::db::schema::PRAGMAS_SEARCH`]'s page cache + /// worth having: searches run on every character typed, so the second + /// query of a session and every one after it finds the b-tree interior + /// pages and FTS5 segment tips already resident. Opening per request threw + /// that away each time. + /// + /// It is reopened when either half of what it was opened against has + /// changed. The path moves when the user points the config at a different + /// index. The epoch moves when the file at the *same* path is replaced — + /// a rebuild, a clear, or a schema-drift wipe — which no comparison of + /// paths could ever catch, and which would otherwise leave this worker + /// querying a deleted inode and pinning its blocks on disk. + fn take_connection(&mut self, db_path: &Path) -> Result { + let epoch = db::index_epoch(); + if let Some(open) = self.open.take() { + if open.epoch == epoch && open.path == db_path { + return Ok(open); + } + // Dropped here, before the open below, so the handle on the old + // index is gone before a handle on the new one exists. + drop(open); + } + Ok(OpenIndex { + conn: db::open::open_search_reader(&db_path.to_string_lossy())?, + epoch, + path: db_path.to_path_buf(), + }) + } + fn send(&self, update: SearchUpdate) { let _ = self.update_tx.send(update); (self.notify)(); } - fn handle(&self, req: SearchRequest) { + fn handle(&mut self, req: SearchRequest) { let generation = req.generation; self.send(SearchUpdate::Started { generation }); @@ -307,9 +418,7 @@ impl Worker { }; let db_path = self.db_path.lock().unwrap().clone(); - // Per-request open: microseconds, and always sees a freshly - // rebuilt index file rather than pinning a deleted inode. - let conn = match db::open_existing(&db_path.to_string_lossy(), false) { + let open = match self.take_connection(&db_path) { Ok(c) => c, Err(e) => { self.send(SearchUpdate::Error { @@ -322,13 +431,13 @@ impl Worker { // Publish the handle tagged with the generation it kills, before the // first statement runs. Anyone cancelling from here on can tell this // search apart from the one that supersedes it. - *self.in_flight.lock().unwrap() = Some((generation, conn.get_interrupt_handle())); + *self.in_flight.lock().unwrap() = Some((generation, open.conn.get_interrupt_handle())); let mut sink = |hits: Vec| { self.send(SearchUpdate::Hits { generation, hits }); }; let outcome = cascade::run( - &conn, + &open.conn, &split, &req.options, generation, @@ -338,6 +447,15 @@ impl Worker { *self.in_flight.lock().unwrap() = None; + // Kept only if it still works. A failed cascade may have failed + // *because* of this connection — a torn file, or an interrupt that + // left it unusable — and putting it back would wedge every search + // after this one behind the same bad handle. Dropping it costs one + // reopen. + if outcome.is_ok() { + self.open = Some(open); + } + match outcome { Ok(Some(Outcome { total, limited })) => self.send(SearchUpdate::Completed { generation, diff --git a/crates/quicksearch-core/src/walk.rs b/crates/quicksearch-core/src/walk.rs index 3d5972e..83f53f0 100644 --- a/crates/quicksearch-core/src/walk.rs +++ b/crates/quicksearch-core/src/walk.rs @@ -29,7 +29,7 @@ use std::fs; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering}; use std::sync::{mpsc, Arc, Condvar, Mutex}; -use std::thread::{self, JoinHandle}; +use std::thread::JoinHandle; use std::time::UNIX_EPOCH; use sha2::{Digest, Sha256}; @@ -1170,7 +1170,7 @@ pub fn walk_indexable_files( let handles = (0..threads) .map(|_| { let (shared, ctx, tx) = (shared.clone(), ctx.clone(), tx.clone()); - thread::spawn(move || { + crate::platform::spawn_worker("qs-walk", move || { // Walker threads are the bulk of a run's CPU and I/O; the // foreground must stay ahead of them. crate::platform::set_background_priority(); @@ -1185,7 +1185,7 @@ pub fn walk_indexable_files( let prefetch = { let (shared, db_path) = (shared.clone(), db_path.to_string()); - thread::spawn(move || { + crate::platform::spawn_worker("qs-prefetch", move || { crate::platform::set_background_priority(); prefetcher(&shared, &db_path) }) diff --git a/crates/quicksearch-core/src/watcher.rs b/crates/quicksearch-core/src/watcher.rs index 4c3ee07..764a287 100644 --- a/crates/quicksearch-core/src/watcher.rs +++ b/crates/quicksearch-core/src/watcher.rs @@ -775,6 +775,13 @@ fn prune_stale(throttle: &mut HashMap, max_age: Durat || entry.immediate || now.saturating_duration_since(entry.record_time) < max_age }); + // `retain` frees the entries but keeps the table sized for the busiest + // moment this map has ever seen — and a build or an unpack can put tens of + // thousands of directories through it in a burst, once, on a process that + // then sits idle for days. + if throttle.is_empty() { + throttle.shrink_to_fit(); + } } #[cfg(test)] diff --git a/crates/quicksearch-core/tests/cascade.rs b/crates/quicksearch-core/tests/cascade.rs index 3154a0f..0934878 100644 --- a/crates/quicksearch-core/tests/cascade.rs +++ b/crates/quicksearch-core/tests/cascade.rs @@ -1072,6 +1072,213 @@ fn service_reports_missing_db_as_error() { service.shutdown(); } +/// Collect the names one search returns, or the error it produced. +/// +/// The worker now keeps its connection between requests, so every test below +/// runs *several* searches against one service — which is the situation the +/// reuse has to survive, and the reason these are written against the service +/// rather than against `cascade::run`. +fn search_names( + service: &SearchService, + updates: &std::sync::mpsc::Receiver, + query: &str, +) -> Result, String> { + let generation = service.search(query, SearchOptions::default()); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + let mut names = Vec::new(); + while std::time::Instant::now() < deadline { + match updates.recv_timeout(std::time::Duration::from_millis(200)) { + Ok(SearchUpdate::Hits { generation: g, hits }) if g == generation => { + names.extend(hits.into_iter().map(|h| h.name)); + } + Ok(SearchUpdate::Completed { generation: g, .. }) if g == generation => { + names.sort(); + return Ok(names); + } + Ok(SearchUpdate::Error { + generation: g, + message, + }) if g == generation => return Err(message), + Ok(_) => {} + Err(_) => break, + } + } + panic!("search {:?} never completed", query); +} + +/// The connection is reused across requests, so a service that has already +/// searched must keep answering correctly rather than serving whatever its +/// first query happened to warm. +#[test] +fn repeated_searches_on_one_service_stay_correct() { + let p = tmp_db("warmrepeat"); + let mut s = Seeder::new(&p, true); + s.add("alpha.txt", "/d", 1, Some("alpha body")); + s.add("beta.txt", "/d", 1, Some("beta body")); + drop(s.done()); + + let (service, updates) = SearchService::new(p.clone(), Arc::new(|| {})); + for _ in 0..3 { + assert_eq!( + search_names(&service, &updates, "alpha").unwrap(), + vec!["alpha.txt"] + ); + assert_eq!( + search_names(&service, &updates, "beta").unwrap(), + vec!["beta.txt"] + ); + } + service.shutdown(); + + std::fs::remove_file(&p).ok(); +} + +/// The hazard the old per-request open made impossible, and the reason +/// `db::index_epoch` exists: a rebuild replaces the file at the *same path*, +/// so nothing about the path tells a held connection that it is now looking at +/// a deleted inode. +#[test] +fn a_rebuilt_index_at_the_same_path_is_picked_up() { + let p = tmp_db("warmrebuild"); + let mut s = Seeder::new(&p, true); + s.add("before.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + let (service, updates) = SearchService::new(p.clone(), Arc::new(|| {})); + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["before.txt"], + "the pre-rebuild index answers first" + ); + + // A rebuild, spelled the way the coordinator spells one: delete the file + // and recreate it. `open_or_recreate` on the now-missing path takes its + // wipe branch, which is the real call site that bumps the epoch — so this + // exercises the actual invalidation rather than poking the counter. + for suffix in ["", "-wal", "-shm"] { + std::fs::remove_file(format!("{}{}", p.display(), suffix)).ok(); + } + let mut s = Seeder::new(&p, true); + s.add("after.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["after.txt"], + "a held connection served the replaced index" + ); + service.shutdown(); + + std::fs::remove_file(&p).ok(); +} + +/// The other half of invalidation: the config points somewhere else entirely. +#[test] +fn set_db_path_repoints_a_held_connection() { + let first = tmp_db("warmpath1"); + let mut s = Seeder::new(&first, true); + s.add("infirst.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + let second = tmp_db("warmpath2"); + let mut s = Seeder::new(&second, true); + s.add("insecond.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + let (service, updates) = SearchService::new(first.clone(), Arc::new(|| {})); + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["infirst.txt"] + ); + + service.set_db_path(second.clone()); + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["insecond.txt"], + "the connection stayed on the old index after the path moved" + ); + service.shutdown(); + + std::fs::remove_file(&first).ok(); + std::fs::remove_file(&second).ok(); +} + +/// A failed search drops the connection rather than putting it back, so one +/// bad handle cannot wedge every search that follows it. +#[test] +fn a_failed_search_does_not_wedge_the_next_one() { + let p = tmp_db("warmrecover"); + let (service, updates) = SearchService::new(p.clone(), Arc::new(|| {})); + + search_names(&service, &updates, "anything").expect_err("no index yet"); + + let mut s = Seeder::new(&p, true); + s.add("arrived.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["arrived.txt"], + "the failure left the worker unable to open the index that appeared" + ); + service.shutdown(); + + std::fs::remove_file(&p).ok(); +} + +/// The connection is released once a search session goes quiet. +/// +/// Not about memory here — it is about the two things an open handle costs +/// that have nothing to do with the page cache: a deleted index keeps its +/// blocks until the last handle closes, and an open reader stops SQLite from +/// resetting the WAL. Checked through `/proc/self/fd` because "the file is no +/// longer open" is the actual claim, and a reopen-still-works assertion would +/// pass whether or not anything was ever released. +#[cfg(target_os = "linux")] +#[test] +fn the_connection_is_released_once_searching_stops() { + let p = tmp_db("warmrelease"); + let mut s = Seeder::new(&p, true); + s.add("held.txt", "/d", 1, Some("shared body")); + drop(s.done()); + + let holds_index = || { + let Ok(entries) = std::fs::read_dir("/proc/self/fd") else { + return false; + }; + entries + .filter_map(|e| e.ok()) + .filter_map(|e| std::fs::read_link(e.path()).ok()) + .any(|target| target == p) + }; + + let (service, updates) = SearchService::new_with_idle_release( + p.clone(), + Arc::new(|| {}), + std::time::Duration::from_millis(150), + ); + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["held.txt"] + ); + assert!(holds_index(), "the connection should be held across requests"); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while holds_index() && std::time::Instant::now() < deadline { + std::thread::sleep(std::time::Duration::from_millis(50)); + } + assert!(!holds_index(), "the idle connection was never released"); + + // And the release must not be terminal: the next search reopens. + assert_eq!( + search_names(&service, &updates, "shared").unwrap(), + vec!["held.txt"] + ); + service.shutdown(); + + std::fs::remove_file(&p).ok(); +} + /// The point of the whole change: a pass hands hits over *while* it scans, so /// the UI has something to show long before the scan ends. /// diff --git a/crates/quicksearch-core/tests/search_perf.rs b/crates/quicksearch-core/tests/search_perf.rs new file mode 100644 index 0000000..43c2313 --- /dev/null +++ b/crates/quicksearch-core/tests/search_perf.rs @@ -0,0 +1,249 @@ +//! What a warm page cache is worth to search, and how big it has to be. +//! +//! The search worker holds one connection across requests (see +//! [`quicksearch_core::search`]) precisely so that a typing session runs +//! against a cache that is already warm. This measures the two things that +//! claim rests on: +//! +//! 1. **Warm beats cold**, by enough to justify holding the connection at all. +//! 2. **8 MiB is enough** — the size [`PRAGMAS_SEARCH`] settles on. The hot set +//! across queries is the `files` b-tree interior nodes and the tips of the +//! FTS5 segments, not the table, so past some point a larger ceiling buys +//! nothing and only raises what an idle process is holding. +//! +//! Queries are run as a *sequence* — `q`, `qu`, `qui`, `quic` — because that is +//! what a search-per-keystroke frontend actually does. The first is the +//! outlier; the second and later ones are the number that matters. +//! +//! The encrypted column is the one to watch. Under SQLCipher a page cache miss +//! costs an AES-CBC decrypt plus an HMAC-SHA512 verify per 4 KiB page rather +//! than a `memcpy`, so if a smaller cache is going to hurt anywhere it is here. +//! +//! Gated by `QSB_SEARCH_PERF` so the harness doesn't pay the seed cost on every +//! `cargo test`. To run it: +//! +//! ```text +//! QSB_SEARCH_PERF=1 cargo test --release -p quicksearch-core \ +//! --test search_perf -- --nocapture +//! ``` + +use std::time::{Duration, Instant}; + +use quicksearch_core::db::repo::{insert_file, set_content_done, NewFile}; +use quicksearch_core::db::{open_or_recreate, set_process_key}; +use quicksearch_core::mime::FileType; +use quicksearch_core::query::split::split_for_cascade; +use quicksearch_core::search::{cascade, SearchHit, SearchOptions}; +use quicksearch_core::security::IndexKey; +use rusqlite::Connection; + +mod common; +use common::scratch_db; + +/// Rows to seed. Large enough that the `files` b-tree has real interior levels +/// and the FTS index has more than one segment — below that everything fits in +/// any cache and the comparison says nothing. +const NUM_FILES: usize = 200_000; + +/// Cache ceilings to compare, as `PRAGMA cache_size` values in KiB. +/// +/// `-40960` is what every read connection used to take, `-8192` is +/// `PRAGMAS_SEARCH`, and `-1024` is deliberately too small — it is there to +/// show the curve has a floor worth being above, so that "8 MiB is enough" is +/// a measurement rather than an assumption. +const CACHE_SIZES: [i64; 6] = [-40960, -32768, -16384, -8192, -4096, -1024]; + +/// The prefixes of one word, typed one character at a time. +const SEQUENCE: [&str; 4] = ["quar", "quart", "quartz", "quartzi"]; + +fn enabled() -> bool { + std::env::var("QSB_SEARCH_PERF").is_ok() +} + +/// Deterministic pseudo-random word picker. Same LCG as `indexprobe`, for the +/// same reason: a fixed seed makes two runs comparable. +struct Lcg(u64); + +impl Lcg { + fn next(&mut self) -> u64 { + self.0 = self.0.wrapping_mul(6364136223846793005).wrapping_add(1); + self.0 >> 33 + } +} + +const WORDS: &[&str] = &[ + "alpha", "beta", "gamma", "delta", "epsilon", "zeta", "eta", "theta", "iota", "kappa", + "lambda", "quartz", "quartzite", "quarry", "quarter", "quantum", "brown", "fox", "jumps", + "lazy", "index", "search", "cascade", "snippet", "document", "content", "extract", +]; + +/// Seed an index with `NUM_FILES` rows, a tenth of them content-indexed. +/// +/// Only a tenth so the FTS index stays smaller than the table, which is the +/// real shape — most files in a tree are not text. +fn seed(path: &std::path::Path) { + let mut conn = open_or_recreate(path.to_str().unwrap(), "trigram").unwrap(); + let mut rng = Lcg(0x5eed); + let tx = conn.transaction().unwrap(); + for i in 0..NUM_FILES { + let w1 = WORDS[(rng.next() as usize) % WORDS.len()]; + let w2 = WORDS[(rng.next() as usize) % WORDS.len()]; + let name = format!("{}-{}-{:07}.txt", w1, w2, i); + let dir = format!("/seed/{:03}", i % 500); + let full = format!("{}/{}", dir, name); + let id = insert_file( + &tx, + &NewFile { + name: &name, + path: &full, + parent: &dir, + size: 4096, + mtime: 1_700_000_000 + i as u64, + inode: None, + device_id: None, + mime: Some("text/plain"), + ftype: FileType::TEXT, + hash: None, + needs_content: i % 10 == 0, + }, + ) + .unwrap() + .expect("unique path"); + if i % 10 == 0 { + let body: Vec<&str> = (0..60) + .map(|_| WORDS[(rng.next() as usize) % WORDS.len()]) + .collect(); + set_content_done(&tx, id, &name, &body.join(" "), &[], true).unwrap(); + } + } + tx.commit().unwrap(); + conn.execute_batch("PRAGMA wal_checkpoint(TRUNCATE);").ok(); +} + +/// Run one query to completion, returning how long it took and how many hits +/// it produced. Hits are counted, not kept — the cost being measured is the +/// scan, and holding 200k `SearchHit`s would measure the allocator instead. +fn time_query(conn: &Connection, query: &str) -> (Duration, usize) { + let split = split_for_cascade(query).unwrap(); + let latest = std::sync::atomic::AtomicU64::new(1); + let mut count = 0usize; + let mut sink = |hits: Vec| count += hits.len(); + let options = SearchOptions { + limit: 1000, + ..SearchOptions::default() + }; + let start = Instant::now(); + cascade::run(conn, &split, &options, 1, &latest, &mut sink).unwrap(); + (start.elapsed(), count) +} + +/// Open a connection at an explicit cache ceiling. +/// +/// Spelled out rather than going through `db::open::open_search_reader` +/// because the whole point is to compare ceilings, which that function +/// deliberately does not expose. +fn open_at(path: &std::path::Path, cache_size: i64) -> Connection { + let conn = Connection::open(path).unwrap(); + conn.execute_batch(&format!( + "PRAGMA busy_timeout = 5000; + PRAGMA cache_size = {}; + PRAGMA temp_store = MEMORY; + PRAGMA foreign_keys = ON;", + cache_size + )) + .unwrap(); + conn +} + +fn run_matrix(label: &str, path: &std::path::Path) { + println!("\n=== {} ===", label); + println!( + "{:>12} {:>10} {:>10} {:>10} {:>8}", + "cache_size", "cold", "warm avg", "warm best", "hits" + ); + for cache_size in CACHE_SIZES { + // A connection per ceiling, held for the whole sequence — the same + // lifetime the search worker gives it. + let conn = open_at(path, cache_size); + let (cold, hits) = time_query(&conn, SEQUENCE[0]); + let mut warm = Vec::new(); + for query in &SEQUENCE[1..] { + warm.push(time_query(&conn, query).0); + } + let avg = warm.iter().sum::() / warm.len() as u32; + let best = warm.iter().min().copied().unwrap_or_default(); + println!( + "{:>12} {:>9.1?} {:>9.1?} {:>9.1?} {:>8}", + cache_size, cold, avg, best, hits + ); + } +} + +/// The headline comparison, printed rather than asserted. +/// +/// Deliberately not a pass/fail threshold: timings on a shared CI box are not +/// stable enough for one, and a flaky perf gate gets muted rather than fixed. +/// This exists to be *read* when the number in [`PRAGMAS_SEARCH`] is being +/// chosen or questioned. +#[test] +fn cache_size_against_search_latency() { + if !enabled() { + eprintln!("skipping: set QSB_SEARCH_PERF=1 to run"); + return; + } + + let plain = scratch_db("searchperf-plain"); + let seeded = Instant::now(); + seed(&plain); + println!( + "seeded {} rows in {:.1?} ({} MiB on disk)", + NUM_FILES, + seeded.elapsed(), + std::fs::metadata(&plain).map(|m| m.len()).unwrap_or(0) / (1024 * 1024) + ); + run_matrix("unencrypted", &plain); +} + +/// The same matrix against an encrypted index. +/// +/// Separate test, and separate process-wide key, because +/// [`set_process_key`] is global: running both in one test would have the +/// plain index opened with a key set. +#[test] +fn cache_size_against_search_latency_encrypted() { + if !enabled() { + eprintln!("skipping: set QSB_SEARCH_PERF=1 to run"); + return; + } + + set_process_key(Some( + IndexKey::from_hex(&"42".repeat(32)).expect("valid 32-byte key"), + )); + let enc = scratch_db("searchperf-enc"); + seed(&enc); + + println!("\n(encrypted: every cache miss costs an AES-CBC + HMAC-SHA512 per page)"); + // Both orders. The ceilings are tried largest-first and then + // smallest-first because the OS page cache warms as the run proceeds, and + // a difference that survives reversing the order is a property of the + // ceiling rather than of when it was measured. + let mut order: Vec = CACHE_SIZES.to_vec(); + order.extend(CACHE_SIZES.iter().rev()); + // Opened through the keyed path — a raw `Connection::open` cannot read it. + for cache_size in order { + let conn = quicksearch_core::db::open_existing(&enc.to_string_lossy(), false).unwrap(); + conn.execute_batch(&format!("PRAGMA cache_size = {};", cache_size)) + .unwrap(); + let (cold, hits) = time_query(&conn, SEQUENCE[0]); + let mut warm = Vec::new(); + for query in &SEQUENCE[1..] { + warm.push(time_query(&conn, query).0); + } + let avg = warm.iter().sum::() / warm.len() as u32; + println!( + "{:>12} cold {:>9.1?} warm avg {:>9.1?} hits {}", + cache_size, cold, avg, hits + ); + } + set_process_key(None); +} diff --git a/crates/quicksearch-gui/src/app.rs b/crates/quicksearch-gui/src/app.rs index d65cd8a..3e67cb3 100644 --- a/crates/quicksearch-gui/src/app.rs +++ b/crates/quicksearch-gui/src/app.rs @@ -2,9 +2,8 @@ //! status bar, and config-change routing. use std::sync::mpsc; -use std::time::{Duration, Instant}; +use std::time::Duration; -use quicksearch_core::cli::IndexCounts; use quicksearch_core::config::{diff_actions, nested_roots, Config, SecurityConfig}; use quicksearch_core::coordinator::{IndexMode, IndexerState, ReconcileState, WatcherStatus}; use quicksearch_core::db; @@ -102,8 +101,6 @@ pub struct QuickSearchApp { dups: DuplicatesTab, logs: LogsTab, options: OptionsWindow, - /// Cached idle counts for the status bar, refreshed at most every 5 s. - counts: Option<(Instant, IndexCounts)>, /// Set when applying a config that invalidates the stored index. rebuild_prompt: Option>, /// Set while the "delete the index?" confirmation is open. @@ -236,7 +233,6 @@ impl QuickSearchApp { dups: DuplicatesTab::new(), logs: LogsTab::new(), options: OptionsWindow::new(), - counts: None, rebuild_prompt: None, clear_prompt: false, nested_prompt, @@ -312,7 +308,6 @@ impl QuickSearchApp { self.backend .search() .set_db_path(new.resolved_database_path()); - self.counts = None; } // Everything the index can reconcile in place — pruning rows a // narrowed filter put out of scope, re-deciding extracted text, @@ -366,28 +361,6 @@ impl QuickSearchApp { self.search .apply_update(update, self.cfg.search.display_limit); } - // Status-bar counts worker. - if let Some(rx) = &self.backend.counts_job { - match rx.try_recv() { - Ok(counts) => { - self.counts = Some((Instant::now(), counts)); - self.backend.counts_job = None; - } - Err(mpsc::TryRecvError::Empty) => {} - // The worker gave up (missing or unreadable index). Keep the - // last known figures but restamp them, so the next attempt - // waits its turn instead of respawning a thread every frame. - Err(mpsc::TryRecvError::Disconnected) => { - let last = self.counts.map(|(_, c)| c).unwrap_or(IndexCounts { - files: 0, - content_done: 0, - content_pending: 0, - }); - self.counts = Some((Instant::now(), last)); - self.backend.counts_job = None; - } - } - } // Duplicates worker. if let Some(rx) = &self.backend.dup_job { match rx.try_recv() { @@ -532,17 +505,7 @@ impl QuickSearchApp { IndexMode::ManualStopped => "Manual", IndexMode::ManualRunning => "Manual", }; - let stale = self - .counts - .map(|(at, _)| at.elapsed() > Duration::from_secs(5)) - .unwrap_or(true); - if stale { - // Kicked off, not awaited: the result lands through - // `drain_events` on a later frame. - let cfg = self.cfg.clone(); - self.backend.start_index_counts(&cfg, ctx.clone()); - } - let files = self.counts.map(|(_, c)| c.files).unwrap_or(0); + let files = state.files.unwrap_or(0); ui.label( egui::RichText::new(format!( "Idle · {} · {} files indexed", @@ -930,7 +893,6 @@ impl QuickSearchApp { } db::set_process_key(new_key); self.backend.coordinator.rebuild_index(); - self.counts = None; self.dups.state = DupState::NotLoaded; } } @@ -1010,7 +972,6 @@ impl QuickSearchApp { if stale_index_window(ctx, self.key_source) { self.stale_index_prompt = false; self.backend.coordinator.rebuild_index(); - self.counts = None; self.dups.state = DupState::NotLoaded; } } @@ -1153,7 +1114,6 @@ impl QuickSearchApp { // launch must not undo that either. self.set_index_mode(false); self.backend.coordinator.clear_index(); - self.counts = None; self.dups.state = DupState::NotLoaded; close = true; } @@ -1294,7 +1254,10 @@ impl QuickSearchApp { /// search runs and the results table clears. pub(crate) fn capture_clear_query(&mut self) { self.search.query.clear(); - self.search.pending_edit = Some(Instant::now()); + // Qualified rather than imported: this is the only `Instant` left in + // this module, and it is behind `feature = "capture"`, so a plain + // `use` would warn in every default build. + self.search.pending_edit = Some(std::time::Instant::now()); } pub(crate) fn capture_focus_search(&mut self) { diff --git a/crates/quicksearch-gui/src/backend.rs b/crates/quicksearch-gui/src/backend.rs index c45901a..912be1a 100644 --- a/crates/quicksearch-gui/src/backend.rs +++ b/crates/quicksearch-gui/src/backend.rs @@ -4,10 +4,16 @@ //! searches stream over an mpsc receiver drained each frame, indexing //! state is polled, and the duplicates query runs on a throwaway worker //! thread. Core threads wake the UI through `ctx.request_repaint()`. +//! +//! The duplicates scan is the *only* throwaway thread left here, and it fires +//! on a user action rather than a timer. The status bar's file count used to +//! spawn one every five seconds, each opening its own connection — a page +//! cache and a fresh allocator arena per refresh, neither of which glibc gives +//! back. It now rides on `IndexerState`, published by the coordinator off the +//! connection it already holds. use std::sync::{mpsc, Arc}; -use quicksearch_core::cli::IndexCounts; use quicksearch_core::config::Config; use quicksearch_core::coordinator::IndexCoordinator; use quicksearch_core::search::{DuplicateGroup, SearchService, SearchUpdate}; @@ -18,8 +24,6 @@ pub struct Backend { pub search: Option, pub search_rx: mpsc::Receiver, pub dup_job: Option, String>>>, - /// In-flight status-bar count; see [`Backend::start_index_counts`]. - pub counts_job: Option>, } impl Backend { @@ -40,35 +44,9 @@ impl Backend { search: Some(search), search_rx, dup_job: None, - counts_job: None, }) } - /// Refresh the status bar's "N files indexed" on a worker thread. - /// - /// Three `COUNT(*)` scans, and on a multi-million-row index the unfiltered - /// one alone reads the whole primary key. Running it inline in `update()` - /// froze a frame every refresh for a number that is purely decorative — so - /// it goes the same way the duplicates scan does. No-op while one is - /// already in flight. - pub fn start_index_counts(&mut self, config: &Config, ctx: egui::Context) { - if self.counts_job.is_some() { - return; - } - let (tx, rx) = mpsc::channel(); - let db = config.resolved_database_path(); - std::thread::spawn(move || { - // A missing or unreadable index is not worth reporting here: the - // status bar has nothing useful to say about it that the indexing - // state does not already say. - if let Ok(counts) = quicksearch_core::cli::index_counts(&db.to_string_lossy()) { - let _ = tx.send(counts); - } - ctx.request_repaint(); - }); - self.counts_job = Some(rx); - } - pub fn search(&self) -> &SearchService { self.search.as_ref().expect("search service alive") } diff --git a/crates/quicksearch-gui/src/manage_tab.rs b/crates/quicksearch-gui/src/manage_tab.rs index e2742aa..ff1bffa 100644 --- a/crates/quicksearch-gui/src/manage_tab.rs +++ b/crates/quicksearch-gui/src/manage_tab.rs @@ -944,6 +944,7 @@ mod tests { mode: IndexMode::Auto, activity: IndexingStatus::Idle, last_full_index: Some(0), + files: Some(0), queued_events: 0, watcher: WatcherStatus::Active { dirs: 10 }, reconcile: None, diff --git a/crates/quicksearch-gui/src/search_tab.rs b/crates/quicksearch-gui/src/search_tab.rs index 936a333..9a7be3a 100644 --- a/crates/quicksearch-gui/src/search_tab.rs +++ b/crates/quicksearch-gui/src/search_tab.rs @@ -974,29 +974,147 @@ fn snippet_formats(ui: &egui::Ui) -> SnippetFormats { } } +/// The mark on a snippet that starts partway into its window. Named because +/// `first_visible_byte` has to pay for its width in advance. +const SNIPPET_LEAD: &str = "… "; + +/// Append `window[range]` to `job`, highlighting whatever parts of `ranges` +/// (byte offsets into `window`) fall inside it. +/// +/// Shared by the one-line Match cell and the multi-row hover and preview +/// jobs: all three paint a sub-slice of one window with the same highlights, +/// and clipping a range against a slice is exactly the sort of off-by-one +/// that gets fixed in one of two copies. +fn append_marked( + job: &mut LayoutJob, + fmt: &SnippetFormats, + window: &str, + ranges: &[(usize, usize)], + range: std::ops::Range, +) { + let mut cursor = range.start; + for &(a, b) in ranges { + let (a, b) = (a.max(range.start), b.min(range.end)); + if a >= b { + continue; // wholly before or after the slice + } + if a > cursor { + job.append(&window[cursor..a], 0.0, fmt.normal.clone()); + } + job.append(&window[a..b], 0.0, fmt.highlight.clone()); + cursor = b; + } + if cursor < range.end { + job.append(&window[cursor..range.end], 0.0, fmt.normal.clone()); + } +} + +/// The byte offset in `snip.window` that rendering has to start at for the +/// first match to land on a row that survives `max_rows`. `0` — render the +/// whole window — whenever it already does, which is the usual case. +/// +/// epaint lays a job out row by row and simply stops at `wrap.max_rows`, and +/// *every* `\n` costs a row, blank line or not. A snippet window opens a +/// third of its byte budget ahead of the hit, so a couple of hundred bytes of +/// ragged lead-in — indented code, a run of blank lines — spends the whole +/// row budget before layout reaches the match, and the mouseover ends up +/// showing context with nothing in it to be context *for*. +/// +/// The row is measured rather than estimated from a character budget: with a +/// proportional font and word wrapping, characters-per-row is wrong in both +/// directions (`ui_util::middle_elide` documents the same lesson), and an +/// estimate would have to re-derive what epaint already knows about tabs +/// (four spaces wide), `\r` (invisible) and empty paragraphs. The probe is +/// one more galley, memoized by job hash, for the one hovered row per frame. +fn first_visible_byte( + ui: &egui::Ui, + snip: &Snippet, + fmt: &SnippetFormats, + max_rows: usize, + wrap_width: f32, +) -> usize { + let Some(&(match_start, _)) = snip.ranges.first() else { + return 0; // a head-of-file window, with nothing to keep on screen + }; + + // The rendered job pays for a leading mark this probe does not, so the + // probe wraps to a narrower width — a point narrower still, since epaint + // rounds `wrap.max_width` before laying out. Every rendered row then + // holds at least what the probe row starting at the same character held, + // so the match cannot drift *down* a row when the job is rebuilt. + let lead_width = ui.fonts(|f| { + SNIPPET_LEAD + .chars() + .map(|c| f.glyph_width(&fmt.normal.font_id, c)) + .sum::() + }); + let mut probe = LayoutJob::default(); + probe.wrap.max_width = (wrap_width - lead_width - 1.0).max(1.0); + probe.append(&snip.window, 0.0, fmt.normal.clone()); + let galley = ui.fonts(|f| f.layout_job(probe)); + + // Cursors index characters; snippet ranges are byte offsets. epaint + // counts the `\n` that ends a row, so the two spaces line up 1:1. + let cursor = egui::text::CCursor { + index: snip.window[..match_start].chars().count(), + // At a wrap, the character belongs to the row it is drawn on, not + // the one it was pushed off. + prefer_next_row: true, + }; + let match_row = galley.layout_from_cursor(cursor).row; + + // epaint trades a glyph or two off the end of the last visible row for + // its own overflow ellipsis, so a match sitting there only counts as + // visible when there was nothing below it to elide in the first place. + let visible_rows = if galley.rows.len() > max_rows { + max_rows.saturating_sub(1) + } else { + max_rows + }; + if match_row < visible_rows { + return 0; + } + + // Keep a third of the budget as lead-in so the hit is not pinned to the + // top edge — the same shape as the window `snippet::extract` picks. The + // two-row preview strip keeps none, and starts on the match's own row. + let mut cursor = cursor; + for _ in 0..max_rows / 3 { + // `Some(0.0)` asks for the row above, not the character above. + cursor = galley.cursor_up_one_row(&cursor, Some(0.0)).0; + } + let start_char = galley.cursor_begin_of_row(&cursor).index; + snip.window + .char_indices() + .nth(start_char) + .map_or(snip.window.len(), |(i, _)| i) +} + /// Build a highlighted snippet LayoutJob from byte ranges, wrapped to at -/// most `max_rows`. Cheap enough to run per visible row per frame. +/// most `max_rows` and started far enough into the window that the first +/// match survives the cap. Cheap enough to run per visible row per frame. fn snippet_job(ui: &egui::Ui, snip: &Snippet, max_rows: usize) -> LayoutJob { let fmt = snippet_formats(ui); + // The width `ui.label` is about to wrap this job to: in a top-down `Ui` + // it overwrites `wrap.max_width` with exactly `ui.available_width()`. + // Setting it here anyway is what lets `first_visible_byte` — and a test — + // lay the job out and see the rows the user will see. + let wrap_width = ui.available_width(); + let start = first_visible_byte(ui, snip, &fmt, max_rows, wrap_width); + let mut job = LayoutJob::default(); + job.wrap.max_width = wrap_width; job.wrap.max_rows = max_rows; - if max_rows == 1 { - job.wrap.break_anywhere = true; - } - if snip.truncated_start { - job.append("… ", 0.0, fmt.weak.clone()); - } - let mut cursor = 0; - for &(start, end) in &snip.ranges { - if start > cursor { - job.append(&snip.window[cursor..start], 0.0, fmt.normal.clone()); - } - job.append(&snip.window[start..end], 0.0, fmt.highlight.clone()); - cursor = end; - } - if cursor < snip.window.len() { - job.append(&snip.window[cursor..], 0.0, fmt.normal.clone()); + if start > 0 || snip.truncated_start { + job.append(SNIPPET_LEAD, 0.0, fmt.weak.clone()); } + append_marked( + &mut job, + &fmt, + &snip.window, + &snip.ranges, + start..snip.window.len(), + ); if snip.truncated_end { job.append(" …", 0.0, fmt.weak); } @@ -1014,12 +1132,6 @@ fn centered_match_job( whole_field: bool, ) -> LayoutJob { let fmt = snippet_formats(ui); - let font_id = egui::TextStyle::Body.resolve(ui.style()); - let char_width = ui.fonts(|f| f.glyph_width(&font_id, '0')).max(1.0); - let mut budget = ((width_px / char_width) as usize).saturating_sub(2).max(8); - if whole_field { - budget = budget.saturating_sub(2); // room for the brackets - } // Newlines force line breaks even in a one-row LayoutJob, wrecking the // centered single-line cell. Flatten them to spaces — a byte-for-byte @@ -1033,78 +1145,142 @@ fn centered_match_job( .contains(['\n', '\r', '\t']) .then(|| snip.window.replace(['\n', '\r', '\t'], " ")); let window = flattened.as_deref().unwrap_or(&snip.window); - let (start, end) = match snip.ranges.first().copied() { - Some((a, b)) => { - let match_chars = window[a..b].chars().count(); - let side = budget.saturating_sub(match_chars) / 2; - let before = &window[..a]; - let after = &window[b..]; - let before_count = before.chars().count(); - let after_count = after.chars().count(); - // Equal context on both sides; leftover budget from a short - // side flows to the other. - let take_before = (side + side.saturating_sub(after_count)).min(before_count); - let take_after = (side + side.saturating_sub(before_count)).min(after_count); - let start = if take_before == 0 { - a - } else { - before - .char_indices() - .nth_back(take_before - 1) - .map(|(i, _)| i) - .unwrap_or(0) - }; - let end = b + after - .char_indices() - .nth(take_after) - .map(|(i, _)| i) - .unwrap_or(after.len()); - (start, end) + + // The budget is in pixels, summed from the font's own glyph advances, + // because nothing else is holding this cell inside its column: the + // centered-and-justified layout puts egui in Extend mode, which lays the + // job out at infinite width, and `Column::clip` then trims a *centered* + // overflow from both ends at once — silently, and taking the highlighted + // match with it. A character count scaled by one sample glyph overshoots + // by a quarter of the column on ordinary text. + let (start, end, decorate) = ui.fonts(|f| { + let font_id = &fmt.normal.font_id; + let width_of = |c: char| f.glyph_width(font_id, c); + let ellipsis = width_of('…'); + let brackets = if whole_field { + width_of('[') + width_of(']') + } else { + 0.0 + }; + let mut marks = 0.0; + if snip.truncated_start { + marks += ellipsis; } - None => { + if snip.truncated_end { + marks += ellipsis; + } + if fits_within(window, width_px - brackets - marks, width_of) { + return (0, window.len(), true); + } + + // Something has to go, so either end may gain a mark. Reserving for + // a cut that does not happen costs a few points of context; missing + // one overflows the column, which is the failure with no mark to + // show for it. + let budget = width_px - brackets - 2.0 * ellipsis; + let Some(&(a, b)) = snip.ranges.first() else { // No ranges (shouldn't happen for match cells) — head trim. - let end = window - .char_indices() - .nth(budget) - .map(|(i, _)| i) - .unwrap_or(window.len()); - (0, end) + return (0, take_forward(window, 0, budget.max(0.0), width_of), true); + }; + if budget <= 0.0 { + // A column narrower than its own punctuation. Spend every point + // on the hit and drop the decoration: the table's 120pt floor + // keeps this out of reach, but a cell that overflows is clipped + // from both ends without a mark to say so, which is the whole + // failure this budget exists to prevent. + return (a, take_forward(window, a, width_px, width_of), false); } - }; + if !fits_within(&window[a..b], budget, width_of) { + // A hit wider than the whole column — a greedy regex or wildcard + // match. Its beginning is the part that has to survive. + return (a, take_forward(window, a, budget, width_of), true); + } + let match_w: f32 = window[a..b].chars().map(width_of).sum(); + + // Equal context on both sides, grown outward one character at a + // time; whichever side is currently narrower is fed first, so the + // leftover from a short side flows to the other. + let (mut start, mut end) = (a, b); + let (mut before_w, mut after_w) = (0.0f32, 0.0f32); + loop { + let prev = window[..start].chars().next_back(); + let next = window[end..].chars().next(); + let used = before_w + match_w + after_w; + let prev_fits = prev.is_some_and(|c| used + width_of(c) <= budget); + let next_fits = next.is_some_and(|c| used + width_of(c) <= budget); + if !prev_fits && !next_fits { + break; + } + // The preferred side wins when it fits; otherwise the other one + // does, since at least one of them just did. + let take_prev = if before_w <= after_w { + prev_fits + } else { + !next_fits + }; + if take_prev { + let c = prev.expect("prev_fits"); + start -= c.len_utf8(); + before_w += width_of(c); + } else { + let c = next.expect("next_fits"); + end += c.len_utf8(); + after_w += width_of(c); + } + } + (start, end, true) + }); let mut job = LayoutJob::default(); job.wrap.max_rows = 1; job.wrap.break_anywhere = true; - if whole_field { + if whole_field && decorate { job.append("[", 0.0, fmt.weak.clone()); } - if start > 0 || snip.truncated_start { + if decorate && (start > 0 || snip.truncated_start) { job.append("…", 0.0, fmt.weak.clone()); } - let mut cursor = start; - for &(a, b) in &snip.ranges { - let (a, b) = (a.max(start), b.min(end)); - if a >= b || a >= end { - continue; - } - if a > cursor { - job.append(&window[cursor..a], 0.0, fmt.normal.clone()); - } - job.append(&window[a..b], 0.0, fmt.highlight.clone()); - cursor = b; - } - if cursor < end { - job.append(&window[cursor..end], 0.0, fmt.normal.clone()); - } - if end < window.len() || snip.truncated_end { + append_marked(&mut job, &fmt, window, &snip.ranges, start..end); + if decorate && (end < window.len() || snip.truncated_end) { job.append("…", 0.0, fmt.weak.clone()); } - if whole_field { + if whole_field && decorate { job.append("]", 0.0, fmt.weak); } job } +/// Whether the whole of `text` fits in `budget` pixels. Stops at the first +/// character that does not, so measuring a 600-character snippet window +/// against a 200-point column costs no more than the column can hold — and +/// this runs for every visible row, every frame. +fn fits_within(text: &str, budget: f32, width_of: impl Fn(char) -> f32) -> bool { + let mut used = 0.0; + for c in text.chars() { + used += width_of(c); + if used > budget { + return false; + } + } + true +} + +/// The byte offset one past the last character of `text[from..]` that still +/// fits in `budget` pixels. +fn take_forward(text: &str, from: usize, budget: f32, width_of: impl Fn(char) -> f32) -> usize { + let mut end = from; + let mut used = 0.0; + for c in text[from..].chars() { + let w = width_of(c); + if used + w > budget { + break; + } + used += w; + end += c.len_utf8(); + } + end +} + /// Jet-colormap chip color per cascade stage — the rank reads as a /// colorbar: cool blue for the strongest matches, warming through cyan, /// green and yellow to red for the weakest path tiers. Pastel rather than @@ -1566,4 +1742,168 @@ mod tests { "out-of-range stages share the fuzzy-path chip" ); } + + // --- snippet rendering ---------------------------------------------- + + use crate::test_ui::{painted_rows, with_ui}; + + /// The rows `job` actually lays out: what the user sees, as opposed to + /// the text the job was built from. `Galley::text` is the latter — it + /// hands back the whole job, including every row epaint dropped at + /// `wrap.max_rows` — so it cannot see a truncation at all. + fn laid_out_rows(ui: &egui::Ui, job: LayoutJob) -> Vec { + ui.fonts(|f| f.layout_job(job)) + .rows + .iter() + .map(|r| r.text()) + .collect() + } + + /// A content snippet whose lead-in is `lines` short lines — the shape + /// that used to eat the whole row budget before the hit was reached. + /// The lead-in is multi-byte on purpose: snippet ranges are byte + /// offsets while row arithmetic counts characters. + fn ragged_snippet(lines: usize) -> Snippet { + let lead = "café\n".repeat(lines); + Snippet { + ranges: vec![(lead.len(), lead.len() + 6)], + window: format!("{lead}NEEDLE and trailing context"), + truncated_start: true, + truncated_end: true, + } + } + + /// The mouseover exists to show the hit in context. A window whose + /// lead-in is dozens of short lines spends the whole row budget before + /// layout reaches the hit, and the tooltip ends up showing context with + /// nothing in it to be context *for*. + #[test] + fn the_hover_snippet_keeps_the_match_when_the_lead_in_is_all_newlines() { + with_ui(|ui| { + ui.set_max_width(520.0); // what the Match cell's tooltip sets + let snip = ragged_snippet(40); + let rows = laid_out_rows(ui, snippet_job(ui, &snip, 10)); + assert!(rows.len() <= 10, "over the row budget: {rows:#?}"); + assert!( + rows.iter().any(|r| r.contains("NEEDLE")), + "the match never made it on screen: {rows:#?}" + ); + // Trimmed at a line boundary and said so. A start landing mid + // character would read "…afé" — or panic on a byte offset that + // is not a char boundary. + assert_eq!(rows[0], "… café", "{rows:#?}"); + }); + } + + /// Same bug, worse: the preview strip under the table has two rows to + /// spend, so a single stray newline in the lead-in is enough. + #[test] + fn the_preview_strip_keeps_the_match_too() { + with_ui(|ui| { + let snip = ragged_snippet(40); + let rows = laid_out_rows(ui, snippet_job(ui, &snip, 2)); + assert!(rows.len() <= 2, "over the row budget: {rows:#?}"); + assert!( + rows.iter().any(|r| r.contains("NEEDLE")), + "the match never made it on screen: {rows:#?}" + ); + }); + } + + /// A window that already fits is rendered exactly as it arrived: no + /// trimming, and no ellipsis for a trim that did not happen. + #[test] + fn a_snippet_that_fits_is_left_alone() { + with_ui(|ui| { + let snip = Snippet { + window: "alpha beta NEEDLE gamma".into(), + ranges: vec![(11, 17)], + truncated_start: false, + truncated_end: false, + }; + assert_eq!( + laid_out_rows(ui, snippet_job(ui, &snip, 10)), + vec!["alpha beta NEEDLE gamma".to_string()] + ); + }); + } + + /// The Match cell is laid out in Extend mode — egui hands it an infinite + /// wrap width — so nothing but the cell's own budget keeps it inside the + /// column, and an overshoot is clipped on *both* sides with no ellipsis, + /// which in a narrow column eats the highlighted match itself. + #[test] + fn the_match_cell_stays_inside_its_column() { + with_ui(|ui| { + let snip = Snippet { + window: "a long stretch of leading context NEEDLE and a long tail after it".into(), + ranges: vec![(34, 40)], + truncated_start: true, + truncated_end: true, + }; + // Down to widths the column itself cannot reach, so the budget + // degrades rather than overflowing. + for width in [20.0, 60.0, 90.0, 120.0, 150.0, 240.0, 400.0, 4000.0] { + for whole_field in [false, true] { + let job = centered_match_job(ui, &snip, width, whole_field); + let galley = ui.fonts(|f| f.layout_job(job)); + assert!( + galley.size().x <= width, + "{}pt of text in a {width}pt column (whole_field={whole_field}): {:?}", + galley.size().x, + galley.text() + ); + // The Match column is `Column::remainder().at_least(120.0)`, + // so anything that wide has to keep the whole hit; below + // that, only its head can be shown, but it is still the + // hit that gets the room rather than the context. + let kept = if width >= 120.0 { "NEEDLE" } else { "N" }; + assert!( + galley.text().contains(kept), + "the match was budgeted away at {width}pt: {:?}", + galley.text() + ); + } + } + }); + } + + /// End to end: hovering the Match cell puts the hit on screen. The cell + /// paints the match once by itself, so the tooltip is the *second* + /// appearance — asserting on one would pass with the bug present. + #[test] + fn hovering_the_match_cell_shows_the_match_in_the_tooltip() { + let ctx = egui::Context::default(); + // Testing that the tooltip carries the match, not egui's hover timing. + ctx.style_mut(|s| { + s.interaction.tooltip_delay = 0.0; + s.interaction.show_tooltips_only_when_still = false; + }); + let mut tab = tab_with_results(1); + tab.has_snippets = true; + tab.results[0].stage = 6; // a full-text stage: no [brackets] + tab.results[0].snippet = Some(ragged_snippet(40)); + + run_frame(&ctx, &mut tab, vec![]); // settle the table's layout + for y in 40..250 { + // x lands in the Match column, past Name and Path. + let pos = egui::pos2(600.0, y as f32); + let mut out = run_frame(&ctx, &mut tab, vec![egui::Event::PointerMoved(pos)]); + if tab.hovered_row != Some(0) { + continue; + } + // The tooltip is its own area, so it may land a frame behind. + for _ in 0..3 { + let showing = painted_rows(&out) + .iter() + .filter(|r| r.contains("NEEDLE")) + .count(); + if showing >= 2 { + return; + } + out = run_frame(&ctx, &mut tab, vec![]); + } + } + panic!("the match never appeared in the hover tooltip"); + } } diff --git a/crates/quicksearch-gui/src/test_ui.rs b/crates/quicksearch-gui/src/test_ui.rs index 0bd11b2..7f1feef 100644 --- a/crates/quicksearch-gui/src/test_ui.rs +++ b/crates/quicksearch-gui/src/test_ui.rs @@ -40,18 +40,34 @@ pub fn click_at(pos: egui::Pos2) -> Vec { ] } -/// Every text galley painted this frame, each with the rectangle it occupies. -/// -/// Labels carry no widget id worth recording, so reading the shapes back is -/// the only way to check the text a user actually sees — and the only way to -/// find a click target that follows the layout instead of pinning it. -pub fn painted(out: &egui::FullOutput) -> Vec<(String, egui::Rect)> { - fn walk(shape: &egui::epaint::Shape, into: &mut Vec<(String, egui::Rect)>) { +/// A `Ui` from a real (headless) egui pass, so measuring helpers see the same +/// fonts the app paints with — the whole point of `middle_elide` and of the +/// snippet row arithmetic is that they agree with egui's own layout. +pub fn with_ui(f: impl FnOnce(&mut egui::Ui) -> R) -> R { + let ctx = egui::Context::default(); + let mut f = Some(f); + let mut out = None; + let _ = ctx.run(egui::RawInput::default(), |ctx| { + egui::CentralPanel::default().show(ctx, |ui| { + if let Some(f) = f.take() { + out = Some(f(ui)); + } + }); + }); + out.expect("the central panel ran") +} + +/// Every text galley painted this frame, in paint order, each with the +/// rectangle it occupies. +fn painted_galleys(out: &egui::FullOutput) -> Vec<(&std::sync::Arc, egui::Rect)> { + fn walk<'a>( + shape: &'a egui::epaint::Shape, + into: &mut Vec<(&'a std::sync::Arc, egui::Rect)>, + ) { match shape { - egui::epaint::Shape::Text(t) => into.push(( - t.galley.text().to_string(), - egui::Rect::from_min_size(t.pos, t.galley.size()), - )), + egui::epaint::Shape::Text(t) => { + into.push((&t.galley, egui::Rect::from_min_size(t.pos, t.galley.size()))) + } egui::epaint::Shape::Vec(shapes) => { for s in shapes { walk(s, into); @@ -60,11 +76,23 @@ pub fn painted(out: &egui::FullOutput) -> Vec<(String, egui::Rect)> { _ => {} } } - let mut out_text = Vec::new(); + let mut galleys = Vec::new(); for clipped in &out.shapes { - walk(&clipped.shape, &mut out_text); + walk(&clipped.shape, &mut galleys); } - out_text + galleys +} + +/// Every text galley painted this frame, each with the rectangle it occupies. +/// +/// Labels carry no widget id worth recording, so reading the shapes back is +/// the only way to check the text a user actually sees — and the only way to +/// find a click target that follows the layout instead of pinning it. +pub fn painted(out: &egui::FullOutput) -> Vec<(String, egui::Rect)> { + painted_galleys(out) + .into_iter() + .map(|(g, rect)| (g.text().to_string(), rect)) + .collect() } /// Every string painted this frame, in paint order. @@ -72,6 +100,20 @@ pub fn painted_text(out: &egui::FullOutput) -> Vec { painted(out).into_iter().map(|(text, _)| text).collect() } +/// Every *visible* row of every galley painted this frame, in paint order. +/// +/// Not the same thing as [`painted_text`]: a galley's `text()` is the job it +/// was laid out from, including the rows epaint dropped at `wrap.max_rows`. A +/// label that silently truncated away the very thing it was meant to show +/// still reads as complete there; the laid-out rows are the only place the +/// loss is visible. +pub fn painted_rows(out: &egui::FullOutput) -> Vec { + painted_galleys(out) + .into_iter() + .flat_map(|(g, _)| g.rows.iter().map(|r| r.text()).collect::>()) + .collect() +} + /// The centre of `needle`'s galley, as a click target. /// /// The *last* match wins, so a string painted both behind a modal and on it diff --git a/crates/quicksearch-gui/src/ui_util.rs b/crates/quicksearch-gui/src/ui_util.rs index 51bdaad..0fcd184 100644 --- a/crates/quicksearch-gui/src/ui_util.rs +++ b/crates/quicksearch-gui/src/ui_util.rs @@ -255,23 +255,7 @@ mod tests { ignore_pattern_valid, middle_elide, pattern_border, pattern_hint, Cow, INVALID_RED, VALID_GREEN, }; - - /// A `Ui` from a real (headless) egui pass, so `middle_elide` measures - /// with the same fonts the app paints with — the whole point of the - /// helper is that its arithmetic agrees with egui's layout. - fn with_ui(f: impl FnOnce(&mut egui::Ui) -> R) -> R { - let ctx = egui::Context::default(); - let mut f = Some(f); - let mut out = None; - let _ = ctx.run(egui::RawInput::default(), |ctx| { - egui::CentralPanel::default().show(ctx, |ui| { - if let Some(f) = f.take() { - out = Some(f(ui)); - } - }); - }); - out.expect("the central panel ran") - } + use crate::test_ui::with_ui; fn body_font(ui: &egui::Ui) -> egui::FontId { egui::TextStyle::Body.resolve(ui.style())