perf(db): reads no longer wait behind writes; pages answer from cache
A series page took about a second to show its seasons on a phone, every visit, although they were cached. Three things stacked up: - One SQLite connection behind one mutex served the whole app, so every read queued behind every write. The database now has one owner: a writer thread for writes and a pool of read-only WAL connections for reads. synchronous = NORMAL and a busy timeout on every connection. - The listing query built the set of every available item in the database before filtering to the parent (~80 ms on a desktop for a 100k-item cache), then fetched user data one row at a time. It now checks availability per row, uses the hierarchy indexes (1.5 ms on the same benchmark) and batches the user-data lookup. - A cache read that missed the 100 ms fast path was set aside until the server answered. It is now raced against the server; whichever answers first with content wins. On the Fairphone, Frasier's season and episode lists now come from cache in 34-133 ms (was 600-1030 ms waiting on the server). Fixes found on the way, each with a test that failed first: - sync_queue_mutation could return another mutation's row id: the id came from a second trip to the shared connection. insert() reads it in the same job. - save_to_cache switched foreign keys off on the shared connection across its awaits, so concurrent writes ran unchecked. The toggle now lives inside one writer job, and a page is one transaction instead of one commit per row. Also: thumbnail LRU touches no longer block the lookup; unused tokio-rusqlite dropped. Design and invariants in docs/architecture/08-database-design.md (Connection ownership, Listing query shape) and 03-data-flow.md.
This commit is contained in:
@@ -314,57 +314,22 @@ pub struct PlaybackModeManager {
|
||||
|
||||
**Location**: `src-tauri/src/storage/db_service.rs`
|
||||
|
||||
Async database interface wrapping synchronous `rusqlite` to prevent blocking the Tokio runtime:
|
||||
Async database interface over `rusqlite`. `RusqliteService` owns the
|
||||
database: writes run as jobs on one writer thread, reads on a pool of read-only
|
||||
WAL connections, so a read never waits for a write. Callers only see the
|
||||
trait (`execute`, `insert`, `execute_detached`, `query_one` / `query_optional` /
|
||||
`query_many`, `transaction`, `transaction_without_foreign_keys`) and build
|
||||
queries with `Query` + `QueryParam`, which keeps values out of the SQL string.
|
||||
|
||||
```rust
|
||||
#[async_trait]
|
||||
pub trait DatabaseService: Send + Sync {
|
||||
async fn execute(&self, query: Query) -> Result<usize, DatabaseError>;
|
||||
async fn execute_batch(&self, queries: Vec<Query>) -> Result<(), DatabaseError>;
|
||||
async fn query_one<T, F>(&self, query: Query, mapper: F) -> Result<T, DatabaseError>
|
||||
where F: FnOnce(&Row) -> Result<T> + Send + 'static;
|
||||
async fn query_optional<T, F>(&self, query: Query, mapper: F) -> Result<Option<T>, DatabaseError>
|
||||
where F: FnOnce(&Row) -> Result<T> + Send + 'static;
|
||||
async fn query_many<T, F>(&self, query: Query, mapper: F) -> Result<Vec<T>, DatabaseError>
|
||||
where F: Fn(&Row) -> Result<T> + Send + 'static;
|
||||
async fn transaction<F, T>(&self, f: F) -> Result<T, DatabaseError>
|
||||
where F: FnOnce(Transaction) -> Result<T> + Send + 'static;
|
||||
}
|
||||
|
||||
pub struct RusqliteService {
|
||||
connection: Arc<Mutex<Connection>>,
|
||||
}
|
||||
|
||||
impl DatabaseService for RusqliteService {
|
||||
async fn execute(&self, query: Query) -> Result<usize, DatabaseError> {
|
||||
let conn = self.connection.clone();
|
||||
tokio::task::spawn_blocking(move || {
|
||||
// Execute query on blocking thread pool
|
||||
}).await?
|
||||
}
|
||||
// ... other methods use spawn_blocking
|
||||
}
|
||||
```
|
||||
|
||||
**Key Benefits:**
|
||||
- **No Freezing**: All blocking DB ops run in thread pool via `spawn_blocking`
|
||||
- **Type Safety**: `QueryParam` enum prevents SQL injection
|
||||
- **Future Proof**: Easy to swap to native async DB (tokio-rusqlite)
|
||||
- **Testable**: Can mock DatabaseService for tests
|
||||
|
||||
**Usage Pattern:**
|
||||
```rust
|
||||
// Before (blocking - causes UI freeze)
|
||||
let conn = database.connection();
|
||||
let conn = conn.lock().unwrap(); // BLOCKS
|
||||
conn.query_row(...) // BLOCKS
|
||||
|
||||
// After (async - no freezing)
|
||||
let db_service = database.service();
|
||||
let db_service = database.service(); // cheap clone of the shared owner
|
||||
let query = Query::with_params("SELECT ...", vec![...]);
|
||||
db_service.query_one(query, |row| {...}).await // spawn_blocking internally
|
||||
db_service.query_one(query, |row| {...}).await // runs on a reader
|
||||
```
|
||||
|
||||
Ownership model, invariants and the reasons for them:
|
||||
[08-database-design.md → Connection ownership](08-database-design.md#connection-ownership).
|
||||
|
||||
## Component Hierarchy
|
||||
|
||||
```mermaid
|
||||
|
||||
@@ -28,12 +28,17 @@ sequenceDiagram
|
||||
Server->>Conn: mark_unreachable() (debounced)
|
||||
end
|
||||
|
||||
alt Cache returns with content
|
||||
alt Cache answers first with content (inside 100ms, or later but before the server)
|
||||
Cache-->>Hybrid: Result with items
|
||||
Hybrid-->>Rust: Return cache result
|
||||
else Cache timeout or empty
|
||||
Server-->>Hybrid: Fresh result (later)
|
||||
Hybrid->>Cache: save_to_cache() in background
|
||||
else Server answers first, or cache is empty
|
||||
Server-->>Hybrid: Fresh result
|
||||
Hybrid-->>Rust: Return server result
|
||||
else Server fails
|
||||
Cache-->>Hybrid: Whatever the cache has (waited for)
|
||||
Hybrid-->>Rust: Return cache result, else the server error
|
||||
end
|
||||
|
||||
Rust-->>Client: SearchResult
|
||||
@@ -42,11 +47,21 @@ sequenceDiagram
|
||||
```
|
||||
|
||||
**Key Points:**
|
||||
- Cache queries have 100ms timeout for responsiveness
|
||||
- Server queries always run for fresh data
|
||||
- Cache wins if it has meaningful content
|
||||
- Automatic fallback to server if cache is empty/stale
|
||||
- Background cache updates (planned)
|
||||
- Both legs start together. A cache answer with content inside 100 ms
|
||||
(`CACHE_FAST_PATH`) returns at once.
|
||||
- **The deadline does not decide the race.** A cache read still running at
|
||||
100 ms is raced against the server (`HybridRepository::race_slow_cache`), and
|
||||
whichever answers first *with content* wins. It used to be that a read past
|
||||
the deadline was only consulted if the server failed, so a page whose cache
|
||||
read took 150 ms always paid the full server round trip — about a second on a
|
||||
phone, on every visit.
|
||||
- An empty or failed cache answer is not a win; the server decides. A failed
|
||||
server falls back to whatever the cache said, waiting for it if necessary.
|
||||
- On a cache win the server's page is still cached in the background when it
|
||||
arrives, so per-user state (positions, favourites) keeps up.
|
||||
- A `get_items` leg that takes 250 ms or more is logged at INFO with its row
|
||||
count, so a slow page can be attributed to the cache or the server from a
|
||||
device log alone.
|
||||
- **Connectivity side-effect**: each server request feeds the `ConnectivityMonitor`, which is the source of truth for the offline/online banner (see [07-connectivity.md](07-connectivity.md)). A server-answered error (401/404/5xx) still counts as *reachable* — only network failures, sustained past a debounce window, flip the app to offline.
|
||||
|
||||
### Listing order is decided in Rust
|
||||
|
||||
@@ -224,10 +224,11 @@ them:
|
||||
the current user — local path, direct play, item id as media source (a download
|
||||
names no source, so the server served its default, which carries the item's id)
|
||||
— and `HybridRepository::get_playback_info` consults it **first**.
|
||||
- **A slow cache read is waited for, never discarded.** The cache is one SQLite
|
||||
connection behind one mutex, so any write in progress (the catalog sync that
|
||||
starts at every launch, a download finishing) pushes a read past the 100 ms fast
|
||||
path. `get_items`, the library list, genres and playlist items used to discard
|
||||
- **A slow cache read is waited for, never discarded.** Reads no longer queue
|
||||
behind writes (see [Connection ownership](08-database-design.md#connection-ownership)),
|
||||
but a big query, a cold page cache or a busy reader pool can still push a read
|
||||
past the 100 ms fast path. Such a read is raced against the server rather
|
||||
than set aside (see [03-data-flow.md](03-data-flow.md)). `get_items`, the library list, genres and playlist items used to discard
|
||||
such a read, wait for the server, and — offline — return its error over data on
|
||||
disk; "More info" on a downloaded show failed that way. They now start the read
|
||||
with `cache_try` (which keeps it running) and `settle` on it when the server
|
||||
|
||||
@@ -594,6 +594,99 @@ flowchart LR
|
||||
| Episode | ~3 KB | ~100 KB | 300 MB - 2 GB |
|
||||
| Full music library (5000 songs) | ~10 MB | ~250 MB | 25-75 GB |
|
||||
|
||||
## Connection ownership
|
||||
|
||||
`storage::Database` is opened once at startup and owns the database for the
|
||||
life of the app; nothing else opens the file. Everything goes through
|
||||
`Database::service()`, which returns a clone of one `RusqliteService`
|
||||
(`storage/db_service.rs`) — callers never hold a `Connection`.
|
||||
|
||||
```
|
||||
┌─────────────────────────────┐
|
||||
execute / insert / │ writer thread ("db-writer") │ one read-write connection,
|
||||
transaction / ────►│ jobs run in arrival order │ foreign_keys = ON
|
||||
execute_detached └─────────────────────────────┘
|
||||
┌─────────────────────────────┐
|
||||
query_one / │ reader pool (3 connections) │ query_only = ON; WAL gives
|
||||
query_optional / ─►│ via spawn_blocking │ each the last committed
|
||||
query_many └─────────────────────────────┘ snapshot
|
||||
```
|
||||
|
||||
Pragmas: `journal_mode = WAL`, `synchronous = NORMAL`, `busy_timeout = 5 s` on
|
||||
every connection.
|
||||
|
||||
**Why.** It used to be one connection behind one `std::sync::Mutex`. WAL was on,
|
||||
but with a single connection its one benefit — readers running beside a writer —
|
||||
never applied: every read queued behind every write. A series page's background
|
||||
refresh cached hundreds of episodes one autocommit (and one fsync) at a time,
|
||||
and library pages, thumbnail lookups and settings reads all waited behind the
|
||||
pile; the same page measured 1.7 s or 3.95 s depending on what was queued.
|
||||
|
||||
**Invariants a change must keep:**
|
||||
|
||||
- **Writes go to the writer, reads to the pool.** A reader is `query_only`, so a
|
||||
write sent through `query_*` fails loudly rather than racing the writer. A
|
||||
read that must see a write *in the same unit of work* belongs inside the
|
||||
`transaction` closure, which runs on the writer.
|
||||
- **Connection-wide state is set inside one writer job.** `PRAGMA foreign_keys`
|
||||
is per connection and ignored inside a transaction. `save_to_cache` used to
|
||||
switch it off with one `execute` and back on with another, so it stayed off
|
||||
for every other write that ran across the save's awaits (they did — nearly
|
||||
all of 1,600 FK-violating writes got through in the regression test).
|
||||
`transaction_without_foreign_keys` flips it around `BEGIN`/`COMMIT` in a single
|
||||
job. Any future pragma toggle must work the same way.
|
||||
- **An insert's rowid comes from the same job.** `insert()` returns it; there is
|
||||
deliberately no standalone `last_insert_rowid()`, which returned whichever row
|
||||
the last *anyone* inserted.
|
||||
- **Batch writes into one transaction.** A commit is a queue slot on the
|
||||
writer; a page of items is one `transaction`, not one `execute` per row.
|
||||
- **Best-effort bookkeeping does not wait.** `execute_detached` queues a write
|
||||
(the thumbnail LRU access time) in order with the rest and returns
|
||||
immediately; failures are only logged.
|
||||
- **The writer survives a panicking job.** The job's caller gets an error; the
|
||||
loop rolls back any transaction the job left open and restores
|
||||
`foreign_keys = ON`, then carries on.
|
||||
|
||||
**`synchronous = NORMAL`** is corruption-safe in WAL mode and survives an app
|
||||
crash; only a power cut can lose the last few commits. Everything stored is
|
||||
either re-fetchable from the server or (the sync queue, local positions)
|
||||
recoverable at that granularity.
|
||||
|
||||
**In-memory databases** (tests) cannot be shared between connections, so
|
||||
`RusqliteService::new` has no pool and routes reads through the writer — the
|
||||
old serialized behaviour.
|
||||
|
||||
**Not done, and why:** grouping consecutive small `execute` jobs into one
|
||||
commit on the writer. With `synchronous = NORMAL` a commit no longer fsyncs, so
|
||||
the gain is small, and it would change a failed statement's error semantics
|
||||
for the jobs grouped with it.
|
||||
|
||||
### Listing query shape
|
||||
|
||||
`OfflineRepository::get_items` (`items_listing_sql`) is the hot read: every
|
||||
library, series, season and album page goes through it, and it must fit the
|
||||
100 ms cache fast path on a phone.
|
||||
|
||||
- **Availability is checked per row, with `EXISTS`** (cached for browsing,
|
||||
downloaded, or a container with a downloaded child). It used to be a CTE
|
||||
that built the id of *every* available item in the database before filtering
|
||||
to the parent: ~80 ms on a desktop for a 100k-item cache, whatever the parent.
|
||||
- **A parent that is not a library is matched on the hierarchy columns alone**
|
||||
(`parent_id`/`album_id`/`season_id`/`series_id`), which SQLite answers with a
|
||||
multi-index `OR`. Whether the parent is a library is looked up first; the
|
||||
library clause can only match for a library, and inside the same `OR` it
|
||||
forced a full scan.
|
||||
- **`+i.server_id`**: the app never runs `ANALYZE`, and without statistics the
|
||||
planner prefers the `server_id` index — which every row shares — over the
|
||||
hierarchy indexes. The unary `+` takes it out of consideration.
|
||||
- **User data is fetched in batches** (`with_user_data`, one `IN (…)` query per
|
||||
500 rows), not once per row.
|
||||
|
||||
Result: 80 ms → 1.5 ms on the desktop benchmark;
|
||||
`listing_a_non_library_parent_uses_the_hierarchy_indexes` asserts the plan.
|
||||
`storage::tests::write_bench_database` (ignored) writes a phone-sized
|
||||
catalogue for timing queries with the `sqlite3` CLI.
|
||||
|
||||
## Rust Module Structure
|
||||
|
||||
```
|
||||
|
||||
Reference in New Issue
Block a user