diff --git a/.gitignore b/.gitignore index 526fb0e..64bcfa0 100644 --- a/.gitignore +++ b/.gitignore @@ -32,4 +32,7 @@ cookies.txt config.toml # Runtime database (watched status, positions, password hash) — never commit +# (+ the WAL/shm sidecars SQLite creates under journal_mode=WAL) catacomb.db +catacomb.db-wal +catacomb.db-shm diff --git a/Cargo.toml b/Cargo.toml index ad96c14..d89cf93 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,6 +75,14 @@ codegen-units = 1 # runtime; those are less informative without symbols but usable with # addr2line against the unstripped build artifact. strip = "debuginfo" +# No unwinding tables/landing pads: smaller binary, marginally faster hot +# paths. The crash handler (crash.rs panic hook) still fires before the +# abort, so crash logging is preserved. Cargo forces `panic=unwind` for +# test/bench harnesses automatically, so `cargo test --release` and the +# #[cfg(test)] catch_unwind in crash.rs are unaffected. Tradeoff: a panic +# in a parallel scan worker aborts the process instead of one worker — +# acceptable, those workers treat all fallible I/O with .ok() already. +panic = "abort" # ── Debian / Ubuntu package (cargo-deb) ────────────────────────────────────── # `cargo deb` reads this to build a .deb. Runtime deps mirror the PKGBUILD: diff --git a/docs/PACKAGING.md b/docs/PACKAGING.md index ff33d4d..7a06e63 100644 --- a/docs/PACKAGING.md +++ b/docs/PACKAGING.md @@ -12,6 +12,18 @@ scripts/package.sh appimage The script builds the release binary once and reuses it for every format. Output lands in `dist/` (gitignored). +> **Building for your own machine?** Distributed packages must stay +> portable, so the repo never sets `target-cpu`. If you compile locally +> and only run the binary on the build host, you can opt into +> host-specific SIMD (helps the ffmpeg-adjacent hot paths like the +> perceptual-dedup hashing): +> +> ```sh +> RUSTFLAGS="-C target-cpu=native" cargo build --release +> ``` +> +> Don't ship that binary — it will SIGILL on any older CPU. + ## Linux formats ### .deb (Debian / Ubuntu / Mint) diff --git a/docs/superpowers/specs/2026-07-07-runtime-performance-pass-design.md b/docs/superpowers/specs/2026-07-07-runtime-performance-pass-design.md new file mode 100644 index 0000000..53a5741 --- /dev/null +++ b/docs/superpowers/specs/2026-07-07-runtime-performance-pass-design.md @@ -0,0 +1,165 @@ +# Runtime performance pass — Phase 1 (low-risk wins) + +**Date:** 2026-07-07 +**Scope:** Runtime performance only, no file/structure refactoring. Three +target areas were requested (desktop GUI, web server, library/DB/scan). This +phase ships the low-risk, behavior-preserving wins; the one invasive change +(desktop row virtualization) is deferred to its own follow-up. + +## Goal + +Make the app faster without changing observable behavior: + +- Cold-cache library scans (thousands of `info.json` sidecars) commit to + SQLite far fewer times and stop re-parsing SQL per row. +- Every SQLite connection uses WAL + sane durability/concurrency pragmas, + which also removes `SQLITE_BUSY` failures under the parallel scanner + (a stability gain). +- The desktop card-sort path stops allocating lowercased strings inside sort + comparators. +- The release binary drops unwinding overhead (`panic = "abort"`), and a + documented opt-in gives local builders SIMD tuning. + +Non-goals: desktop row virtualization (deferred), any web-handler change (the +web path is already ETag + body-cached and inherits the DB speedups), and any +structural/file refactor. + +## Changes + +### 1. SQLite connection pragmas via `with_init` — `database.rs` + +Today `Database::open` / `open_in_memory` build the pool with no per-connection +initialization, so every pooled connection runs on SQLite defaults +(`journal_mode=DELETE`, `synchronous=FULL`, no `busy_timeout`). Cold-cache +scans therefore fsync once per `info_cache_put`, and concurrent writers from +the parallel scanner can hit `SQLITE_BUSY` immediately instead of waiting. + +Set pragmas on **every** connection through +`SqliteConnectionManager::file(path).with_init(|conn| { ... })`: + +- `PRAGMA journal_mode = WAL;` — better write throughput + reader/writer + concurrency. Creates `catacomb.db-wal` / `catacomb.db-shm` sidecars (SQLite + manages their lifecycle). **Must be gitignored** alongside `catacomb.db`. +- `PRAGMA synchronous = NORMAL;` — safe with WAL; commits append to the WAL + without a full fsync per commit. +- `PRAGMA busy_timeout = 5000;` — wait up to 5s for a lock rather than + failing a concurrent write. Removes the parallel-scanner `SQLITE_BUSY` + failure mode. +- `PRAGMA foreign_keys = ON;` — FK enforcement is **per-connection**. It is + currently set once on a single connection (`init_schema`, ~line 410), which + means most pooled connections run with FKs *off*. Moving it into `with_init` + makes enforcement consistent. The standalone `execute("PRAGMA foreign_keys + = ON")` line in `init_schema` is then removed as redundant. + +The in-memory pool (`open_in_memory`, used by tests) gets the same `with_init` +except **not** WAL (an in-memory DB has no file; WAL is a no-op/undesirable +there) — apply `foreign_keys`, `busy_timeout`, and leave journal/synchronous +at their in-memory defaults. + +`with_init` runs on each new physical connection; because pragmas like +`synchronous`, `busy_timeout`, and `foreign_keys` are per-connection they must +live here, not in a one-shot at open time. `journal_mode=WAL` persists in the +file header but re-asserting it per connection is harmless and idempotent. + +### 2. `prepare_cached` for the per-video scan queries — `database.rs` + +`info_cache_get` and `info_cache_put` call `conn.prepare(...)` / +`conn.execute(...)` — a fresh SQL parse on every one of thousands of videos per +scan. Switch both to `conn.prepare_cached(...)`, which reuses the compiled +statement from the connection's statement cache. Behavior is identical; only +the per-call SQL-compile cost is removed. + +Other one-shot queries (folder loads, settings reads, etc.) are **not** changed +— they run once per operation, so statement caching buys nothing there. Scope +stays on the two functions that run in the per-video inner loop. + +### 3. Batch cold-cache writes in one transaction — `library.rs` + `database.rs` + +`enrich_with_cache` currently upserts each cache **miss** via an individual +auto-committed `info_cache_put`. On a first-ever (cold) scan, every video is a +miss, so a channel with N videos does N separate commits. + +Restructure `enrich_with_cache` so cache misses are collected into a +`Vec<(path, mtime, dur, has_chapters, upload_date)>` while building the +`Video`s, then flushed once via a new +`Database::info_cache_put_many(&[...])` that wraps all inserts in a single +transaction (using a `prepare_cached` statement inside the tx). Cache **hits** +still short-circuit with no write, exactly as today. Net effect: a cold scan of +a channel commits once instead of once-per-video. With WAL+NORMAL already in +place this is a smaller marginal win than it would be on the old journal, but +it keeps cold scans cheap and is a clean, contained change. + +If threading the miss-collection through the existing closure proves to tangle +`enrich_with_cache` readability, fall back to keeping per-row `info_cache_put` +(now `prepare_cached` + WAL, already much cheaper) and drop this item — it is +the lowest-priority change in the pass. + +### 4. Sort without per-comparison allocations — `app.rs` + +In `compute_cards`, the `SortMode::Title` and `SortMode::ChannelAsc` arms call +`.to_lowercase()` inside the `sort_by` comparator, allocating two strings per +comparison (O(n log n) allocations). Replace with `sort_by_cached_key` (or a +precomputed key vector) so each element is lowercased once. This arm only runs +when the sort/search/view key changes (the result is cached by `cards_take`), +so it is not a per-frame cost — but the fix is nearly free and removes a +needless allocation storm on large libraries when the user changes sort. + +### 5. Release-profile compiler settings — `Cargo.toml` + +The `[profile.release]` block is already well-tuned (`opt-level = 3`, +`lto = "thin"`, `codegen-units = 1`, `strip = "debuginfo"`). Full LTO is +deliberately avoided (it broke linking with bundled SQLite + rust-lld — keep +thin). Two runtime-oriented changes: + +- **Add `panic = "abort"`.** Removes unwinding tables / landing pads: smaller + binary, marginally faster hot paths, faster panic→exit. The crash handler + installs a `panic::set_hook` ([crash.rs](../../../src/crash.rs)) which + **still fires** under abort, so crash logging is preserved. `cargo test` + (incl. `cargo test --release`) is unaffected — Cargo automatically forces + `-C panic=unwind` when building test/bench harnesses, so the existing suite + and the only `catch_unwind` (a `#[cfg(test)]` block in `crash.rs`) keep + working. Accepted tradeoff: a panic inside a `parallel_map` scan worker + ([library.rs](../../../src/library.rs)) aborts the whole process instead of + failing just that worker — acceptable because such a panic indicates a + genuine invariant break, and the scan workers only do fallible I/O/JSON work + that already uses `.ok()` rather than panicking. + +- **`target-cpu`: local opt-in only, no repo change.** A committed + `target-cpu=native` / `x86-64-v3` would SIGILL on any CPU older than the + build host and would leak into the CI Windows cross-compile (it's a + `RUSTFLAGS` setting, not a profile key). Instead, document in + [docs/PACKAGING.md](../../../docs/PACKAGING.md) (or the build section of the + README) that users compiling for their own machine can get SIMD gains with + `RUSTFLAGS="-C target-cpu=native" cargo build --release`. Distributed and CI + builds stay portable. + +## Out of scope / deferred + +- **Desktop row virtualization** (`ScrollArea::show_rows` for List/Card/Grid). + Highest per-frame win but changes scroll internals and needs fixed row + heights (title line-clamping). Deferred to a dedicated follow-up so it can be + tested in isolation. +- **Web handlers.** Already ETag + body-cached; no change this pass. + +## Testing / verification + +- `cargo build --release` clean. +- `cargo test --release` — existing `tests/api.rs` integration tests spawn the + real `--web` binary against a throwaway dir and must still pass (this + exercises the new pragmas end-to-end, including WAL sidecar creation). +- Add a unit test for `info_cache_put_many` round-trip (put many → get each + back) if item #3 lands. +- Manual smoke: run against a real library dir, confirm scan completes and the + `catacomb.db-wal` file appears, and a warm rescan is a fast no-op. +- `.gitignore`: add `catacomb.db-wal` and `catacomb.db-shm` (or `catacomb.db*`). +- Confirm `cargo test --release` still builds/passes with `panic = "abort"` + set (Cargo forces unwind for the harness — this is the check that proves it). +- Cross-compile smoke: `cargo check --target x86_64-pc-windows-gnu` stays green + (per CLAUDE.md) — `panic = "abort"` is portable, but verify. + +## Risk + +Low. All four changes preserve observable behavior. The only new on-disk +artifact is the WAL sidecar pair, handled by gitignore. `busy_timeout` and +`foreign_keys` becoming consistent across connections is strictly more correct +than today. diff --git a/src/app.rs b/src/app.rs index 1be1e1f..37cb2f9 100644 --- a/src/app.rs +++ b/src/app.rs @@ -938,7 +938,9 @@ impl App { } match self.sort_mode { - SortMode::Title => cards.sort_by(|a, b| a.title.to_lowercase().cmp(&b.title.to_lowercase())), + // sort_by_cached_key lowercases each element once instead of + // twice per comparison (O(n) allocations, not O(n log n)). + SortMode::Title => cards.sort_by_cached_key(|c| c.title.to_lowercase()), SortMode::DurationAsc => cards.sort_by(|a, b| { a.duration_secs.unwrap_or(0.0) .partial_cmp(&b.duration_secs.unwrap_or(0.0)) @@ -977,9 +979,8 @@ impl App { cards.sort_by(|a, b| a.mtime_unix.unwrap_or(u64::MAX).cmp(&b.mtime_unix.unwrap_or(u64::MAX))); } SortMode::ChannelAsc => { - cards.sort_by(|a, b| { - a.channel_name.to_lowercase().cmp(&b.channel_name.to_lowercase()) - .then_with(|| a.title.to_lowercase().cmp(&b.title.to_lowercase())) + cards.sort_by_cached_key(|c| { + (c.channel_name.to_lowercase(), c.title.to_lowercase()) }); } } diff --git a/src/database.rs b/src/database.rs index f9ecfa3..fc3ecc6 100644 --- a/src/database.rs +++ b/src/database.rs @@ -138,7 +138,25 @@ impl Database { /// hash and resume positions aren't readable by other local users. A /// best-effort: failure is logged but doesn't abort startup. pub fn open(path: &Path) -> Result { - let manager = SqliteConnectionManager::file(path); + // Per-connection pragmas. `synchronous`, `busy_timeout` and + // `foreign_keys` are connection-scoped in SQLite, so they must run on + // every pooled connection (`with_init`), not once at open. + // - WAL: write throughput + readers don't block the writer. Persists + // in the file header; re-asserting per connection is idempotent. + // Creates `catacomb.db-wal`/`-shm` sidecars (gitignored). + // - synchronous=NORMAL: safe with WAL, drops the per-commit fsync. + // - busy_timeout: the parallel scanner's concurrent writers wait for + // the lock instead of failing SQLITE_BUSY immediately. + // - foreign_keys: enforcement is per-connection; without this, only + // whichever connection ran init_schema had it on. + let manager = SqliteConnectionManager::file(path).with_init(|conn| { + conn.execute_batch( + "PRAGMA journal_mode = WAL; + PRAGMA synchronous = NORMAL; + PRAGMA busy_timeout = 5000; + PRAGMA foreign_keys = ON;", + ) + }); let pool = Pool::builder() .max_size(FILE_POOL_SIZE) .build(manager) @@ -168,7 +186,14 @@ impl Database { /// is capped at 1 connection here. Otherwise each `get()` would hand back /// a fresh, empty database and our schema/data would vanish between calls. pub fn open_in_memory() -> Result { - let manager = SqliteConnectionManager::memory(); + // Same per-connection pragmas as `open`, minus WAL/synchronous — + // journaling pragmas are meaningless for an in-memory database. + let manager = SqliteConnectionManager::memory().with_init(|conn| { + conn.execute_batch( + "PRAGMA busy_timeout = 5000; + PRAGMA foreign_keys = ON;", + ) + }); let pool = Pool::builder() .max_size(1) .build(manager) @@ -406,8 +431,7 @@ impl Database { /// "Unfiled". pub fn delete_folder(&self, id: i64) -> Result<()> { let conn = self.conn(); - // Enable FK cascade for this connection — SQLite has it off by default. - conn.execute("PRAGMA foreign_keys = ON", [])?; + // FK cascade is on for every pooled connection (see `with_init`). conn.execute("DELETE FROM folders WHERE id = ?1", [id])?; Ok(()) } @@ -750,7 +774,9 @@ impl Database { mtime_unix: u64, ) -> Option<(Option, bool, Option)> { let conn = self.conn(); - let mut stmt = conn.prepare( + // prepare_cached: this runs once per video per scan — reuse the + // compiled statement instead of re-parsing the SQL each call. + let mut stmt = conn.prepare_cached( "SELECT mtime_unix, duration_secs, has_chapters, upload_date \ FROM info_cache WHERE path = ?1", ).ok()?; @@ -766,30 +792,39 @@ impl Database { Some((dur, chap != 0, date)) } - /// Upsert a parsed info.json result into the cache. Called on miss - /// by the library scanner. Errors are swallowed — a cache miss next - /// time costs the same as no cache at all. - pub fn info_cache_put( + /// Bulk upsert of parsed info.json results in a single transaction. + /// Called with the scanner's cache misses (`enrich_with_cache`). + /// + /// A cold-cache scan misses on every video; committing each row + /// individually costs one WAL append per video. Wrapping the batch in + /// one transaction commits once per channel instead. Errors are + /// swallowed — a lost cache write only means a re-parse next scan. + pub fn info_cache_put_many( &self, - path: &str, - mtime_unix: u64, - duration_secs: Option, - has_chapters: bool, - upload_date: Option<&str>, + rows: &[(String, u64, Option, bool, Option)], ) { - let conn = self.conn(); - let _ = conn.execute( - "INSERT OR REPLACE INTO info_cache \ - (path, mtime_unix, duration_secs, has_chapters, upload_date) \ - VALUES (?1, ?2, ?3, ?4, ?5)", - rusqlite::params![ - path, - mtime_unix as i64, - duration_secs, - has_chapters as i64, - upload_date, - ], - ); + if rows.is_empty() { + return; + } + let mut conn = self.conn(); + let Ok(tx) = conn.transaction() else { return }; + { + let Ok(mut stmt) = tx.prepare_cached( + "INSERT OR REPLACE INTO info_cache \ + (path, mtime_unix, duration_secs, has_chapters, upload_date) \ + VALUES (?1, ?2, ?3, ?4, ?5)", + ) else { return }; + for (path, mtime, dur, chap, date) in rows { + let _ = stmt.execute(rusqlite::params![ + path, + *mtime as i64, + dur, + *chap as i64, + date.as_deref(), + ]); + } + } + let _ = tx.commit(); } /// Idempotently merge another catacomb database into this one. @@ -1230,6 +1265,39 @@ pub struct RestoreSummary { pub notes_added: u64, } +#[cfg(test)] +mod info_cache_tests { + use super::*; + + #[test] + fn put_many_roundtrip_and_mtime_gate() { + let db = Database::open_in_memory().unwrap(); + let rows = vec![ + ("/lib/a.info.json".to_string(), 100u64, Some(12.5), true, Some("20260101".to_string())), + ("/lib/b.info.json".to_string(), 200u64, None, false, None), + ("/lib/c.info.json".to_string(), 300u64, Some(0.0), false, Some("20251231".to_string())), + ]; + db.info_cache_put_many(&rows); + + assert_eq!( + db.info_cache_get("/lib/a.info.json", 100), + Some((Some(12.5), true, Some("20260101".to_string()))) + ); + assert_eq!(db.info_cache_get("/lib/b.info.json", 200), Some((None, false, None))); + // Stale mtime and unknown path both miss. + assert_eq!(db.info_cache_get("/lib/a.info.json", 101), None); + assert_eq!(db.info_cache_get("/lib/nope.info.json", 100), None); + + // Re-put with a new mtime replaces the row (INSERT OR REPLACE). + db.info_cache_put_many(&[("/lib/a.info.json".to_string(), 101u64, Some(99.0), false, None)]); + assert_eq!(db.info_cache_get("/lib/a.info.json", 101), Some((Some(99.0), false, None))); + assert_eq!(db.info_cache_get("/lib/a.info.json", 100), None); + + // Empty batch is a no-op, not an error. + db.info_cache_put_many(&[]); + } +} + #[cfg(test)] mod search_tests { use super::*; diff --git a/src/library.rs b/src/library.rs index ab6b432..b746364 100644 --- a/src/library.rs +++ b/src/library.rs @@ -449,6 +449,10 @@ fn collect_raw_videos(entries: impl Iterator) -> Vec, cache: Option<&Database>) -> Vec