Skip to content

perf: reuse prepared statements in the db layer - #62

Closed
elecnix wants to merge 1 commit into
mainfrom
reuse-prepared-statements-a
Closed

perf: reuse prepared statements in the db layer#62
elecnix wants to merge 1 commit into
mainfrom
reuse-prepared-statements-a

Conversation

@elecnix

@elecnix elecnix commented Aug 9, 2026

Copy link
Copy Markdown
Owner

What this does (plain English)

pi-prospector talks to a small local database that stores your analysis. Every time it wanted to ask the database a question, it would re-teach the database the full wording of that question from scratch — thousands of times in a single run, even for the same question repeated over and over. Repeating that teaching step (SQLite calls it "preparing" a statement) is busier than it looks.

This change lets the program remember each question it has already taught, so repeated questions reuse the remembered version instead of re-teaching it every time. It is pure housekeeping: the answers it gets are exactly the same — it just stops doing the same setup work repeatedly.

Technical detail

Nearly every db access went through db.prepare(), so SQLite re-parsed and re-planned each statement on every call (db.cache is undefined in better-sqlite3). This adds a per-connection prepared-statement cache and routes the hot helpers through it:

  • src/db/statement-cache.ts — a WeakMap<Database, Map<sql, Statement>>. The cache is keyed by the Database instance because a better-sqlite3 Statement is bound to the connection it was prepared on; a module-global SQL→Statement map would hand a statement from a closed temp database to the next test.
  • prepare(db, sql) returns the cached statement for that connection, preparing once and reusing afterwards. Only connections that have run migrate() (and therefore carry the final schema) ever store; a raw connection that skipped migrate() still executes but caches nothing — so it is purely an optimisation and never changes behaviour.
  • initializeStatementCache(db) is called as the final step of migrate(), tying cache lifetime to the connection after schema migration. A statement is never compiled against pre-migration schema.
  • Routed queries.ts, analysis-queries.ts, and the framework's hot db_loadMessages (the sanity check the issue asks for).

Correctness / no-behaviour-change verification

  • npm test: 441 passed, 0 failed (unit + component, mock LLM).
  • Integration (test/integration/test-commands.ts): 21 passed, 0 failed — full sync→analyze→proposal flow, idempotent re-runs, revise-mode lineage, proposal lifecycle, prospect verify.
  • tsc --noEmit clean.
  • New focused suite tests/component/statement-cache.test.ts covers: same statement reused for identical SQL on a connection; statements not shared across connections (a closed connection's cached statement is never handed to another); uninitialized connections still execute but don't cache; migrate() initializes the cache.

Measurement (exactly what and against what)

Measured on this branch's code vs raw per-call db.prepare on the same connection, fresh scratch SQLite DB under /tmp, 200,000 bound lookups of a simple indexed query:

  • cached (helper): ~1565 ms; per-call db.prepare: ~1910 ms → ~1.2×.

This compares the statement-reuse effect directly against main's per-call behaviour on the same connection, so it does not double-count #55 (that PR batched node+edge writes into a single db.transaction(), which is orthogonal). It is not a headline speedup: the whole write path is a fraction of a percent of wall time on a real corpus, and this change is framed as hygiene, not as a wall-time target.

— quiet-sloth-61

better-sqlite3 re-parses and re-plans SQL on every db.prepare() call and
ships no statement cache. Route the hot db helpers in queries.ts,
analysis-queries.ts (and the framework's message loader) through a
per-connection prepared-statement cache: a WeakMap keyed by the Database
instance, so a Statement is never served across connections, and only
migrated (post-schema) connections ever store statements.

Behaviour is unchanged: identical results, idempotent re-runs, cache
created as the final step of migrate().
@elecnix

elecnix commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Closing this in favour of #61a cost decision, and a close one.

Both PRs implemented issue #58 independently: same task, same brief, two different model configurations, neither aware of the other. Both handled all three of the issue's stated warnings, both are green, and both correctly declined to quote a headline speedup.

Where this PR was better: it covered src/analyze/framework.ts, which the issue names as one of three locations and which #61 deliberately left out. That is real scope, and it is the analyzer's hot read path. It has been handed to #61's author to fold in, with instructions to verify the SQL is stable before caching on it rather than copying the change.

Where #61 was better, and why it won on more than price: its cache is a lazily-populated WeakMap keyed by Database, so it needs no initialisation call at all. This PR's explicit initializeStatementCache(db) at the end of migration is well-reasoned — the comment about a cached statement referencing pre-migration schema is exactly right — but it solves a problem the lazy design does not have, and it introduces a step a future connection path could omit. A structural guarantee beat a convention.

On cost: this PR took 2.8× more to produce ($0.297 against $0.106) across 92 requests versus 52, for a larger diff (6 files, +259/−91 against 4 files, +149/−86).

Nothing here is discarded — the framework.ts coverage is the reason #61 will be more complete than it was, and the migration-ordering reasoning is recorded here for anyone who revisits the cache design.

Thank you — this was a genuinely close pair, and the scope call was yours to have got right.

@elecnix elecnix closed this Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant