perf(dotc1z): add prepared statement cache for repeated SQL queries#864
perf(dotc1z): add prepared statement cache for repeated SQL queries#864arreyder wants to merge 3 commits into
Conversation
Profile of be-temporal-sync shows modernc.org/sqlite.(*conn).bind consuming 163s/hr (5.6% CPU) recompiling the same SQL shapes thousands of times per sync. With MaxOpenConns(1), all queries execute on a single connection — ideal for a statement cache. Add a map[string]*sql.Stmt on C1File that lazily caches prepared statements keyed by their SQL text. Since goqu already uses .Prepared(true) on read paths (generating stable ?-placeholder SQL), the SQL text is deterministic for each query shape and serves as a natural cache key. Seven read-path call sites converted from db.QueryContext/ db.QueryRowContext to stmt.QueryContext/stmt.QueryRowContext: - listConnectorObjects (sql_helpers.go) — sync's main list loop - getConnectorObject (sql_helpers.go) — GetEntitlement, GetResource - getResourceObject (sql_helpers.go) — GetResource variant - listGrantsGeneric (grants.go) — ListGrantsForEntitlement - listExpandableGrantsInternal (grants_expandable_query.go) - listGrantsWithExpansionInternal (grants_expandable_query.go) - hydrateSingleGrant (grants_hydrate.go) Write paths (executeChunkedInsert) left unchanged — they already use transactions and the INSERT shapes vary with batch size. Statements are closed in closeRawDB before the database handle is released. Double-check locking in getOrPrepare handles concurrent callers safely (though MaxOpenConns(1) makes true concurrency unlikely in practice). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
General PR Review: perf(dotc1z): add prepared statement cache for repeated SQL queriesBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit adds the nil-guard for Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
…t error Addresses code review findings: 1. If closeRawDB runs (nils stmtCache) and getOrPrepare is called after (stale reference, context-cancelled goroutine), the nil map write would panic. Guard with nil check returning ErrDbNotOpen. 2. Wrap PrepareContext error with function name for debuggability in stack-trace-free logging. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| c.stmtCacheMu.Unlock() | ||
| stmt.Close() | ||
| return existing, nil | ||
| } | ||
| c.stmtCache[query] = stmt | ||
| c.stmtCacheMu.Unlock() |
There was a problem hiding this comment.
🟡 Suggestion: The new nil guard at the top of getOrPrepare prevents a nil-map read on entry, but this second lock section can still write to a nil map if closeRawDB sets stmtCache = nil between the first unlock and this re-lock (the window exists because closeRawDB nils the cache before closing rawDb). Adding a nil check here would make the function fully safe against that shutdown race.
| c.stmtCacheMu.Unlock() | |
| stmt.Close() | |
| return existing, nil | |
| } | |
| c.stmtCache[query] = stmt | |
| c.stmtCacheMu.Unlock() | |
| c.stmtCacheMu.Lock() | |
| if c.stmtCache == nil { | |
| c.stmtCacheMu.Unlock() | |
| stmt.Close() | |
| return nil, ErrDbNotOpen | |
| } | |
| if existing, ok := c.stmtCache[query]; ok { | |
| c.stmtCacheMu.Unlock() | |
| stmt.Close() | |
| return existing, nil | |
| } | |
| c.stmtCache[query] = stmt | |
| c.stmtCacheMu.Unlock() |
Benchmark resultsSame machine, same fixtures as PR #863 benchmarks. This branch has ONLY the stmt cache (no prefetch from #863) — measuring the cache in isolation. 17-18% wall-clock improvement, allocations flat. The savings are pure CPU: eliminating repeated SQLite bytecode compilation for the same ~10-20 query shapes that execute thousands of times per sync. Allocations stay flat because goqu still builds the SQL string each call (used as the cache key) — cutting that is Phase 3 (raw SQL templates), a future PR. This stacks with PR #863 (prefetch): #863 reduces the NUMBER of queries by 10,000x; #864 makes each remaining query faster by ~18%. Together they compound. |
| stmt.Close() | ||
| return existing, nil | ||
| } | ||
| c.stmtCache[query] = stmt |
There was a problem hiding this comment.
[🤖] P1: getOrPrepare can still panic on the cache-miss path if closeRawDB runs between preparing the new statement and re-acquiring stmtCacheMu. closeRawDB sets c.stmtCache = nil, so this write can become a write to a nil map. Please mirror the entry nil check after the second lock, close the newly prepared statement, and return ErrDbNotOpen before checking/storing the cache entry.
If closeRawDB runs between the first unlock (cache-miss path) and the second lock (store path), stmtCache is nil and the map write panics. Add the same nil guard as the first lock section. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
2 participants
Summary
map[string]*sql.Stmtcache on*C1Filethat eliminates repeated SQLite statement compilation for read queriesmodernc.org/sqlite.(*conn).bindconsumes 163s/hr (5.6% CPU) recompiling the same ~10 SQL shapes thousands of times per syncMaxOpenConns(1), all statements are bound to a single connection for the lifetime of the sync activity — ideal for cachingHow it works
goqu already uses
.Prepared(true)on all read paths, generating stable?-placeholder SQL. The SQL text is deterministic for each query shape (determined by which WHERE clauses are active).getOrPrepare(ctx, query)checks the cache; on miss, prepares the statement and stores it. On hit, returns the cached*sql.Stmtimmediately.Call sites converted (7)
sql_helpers.golistConnectorObjectssql_helpers.gogetConnectorObjectsql_helpers.gogetResourceObjectgrants.golistGrantsGenericgrants_expandable_query.golistExpandableGrantsInternalgrants_expandable_query.golistGrantsWithExpansionInternalgrants_hydrate.gohydrateSingleGrantWhat's NOT changed
executeChunkedInsert) — already use transactions, INSERT shapes vary with batch sizeBenchmark
17-18% wall-clock improvement. Allocations flat (goqu still builds the SQL string — savings are pure CPU from eliminating bytecode recompilation).
Test plan
go test -short ./pkg/dotc1z/...— all passgo test ./pkg/sync/...— all passgo vet ./pkg/dotc1z/...— clean🤖 Generated with Claude Code