Skip to content

fix(perf): apply cachedStmt to buildTestFileIds static SQL #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
carlos-alm merged 3 commits into from
Mar 23, 2026
Merged

fix(perf): apply cachedStmt to buildTestFileIds static SQL #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e165c48
fix(perf): apply cachedStmt to buildTestFileIds static SQL (closes #571)
carlos-alm Mar 23, 2026
c74f386
Merge branch 'main' into fix/571-cachedStmt-buildTestFileIds
carlos-alm Mar 23, 2026
aec2d87
Merge branch 'main' into fix/571-cachedStmt-buildTestFileIds
carlos-alm Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions src/domain/analysis/module-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,21 @@
import path from 'node:path';
import { openReadonlyOrFail, testFilterSQL } from '../../db/index.js';
import { cachedStmt } from '../../db/repository/cached-stmt.js';
import { loadConfig } from '../../infrastructure/config.js';
import { debug } from '../../infrastructure/logger.js';
import { isTestFile } from '../../infrastructure/test-filter.js';
import { DEAD_ROLE_PREFIX } from '../../shared/kinds.js';
import type { BetterSqlite3Database } from '../../types.js';
import type { BetterSqlite3Database, StmtCache } from '../../types.js';
import { findCycles } from '../graph/cycles.js';
import { LANGUAGE_REGISTRY } from '../parser.js';

// ---------------------------------------------------------------------------
// Statement caches (one prepared statement per db instance)
// ---------------------------------------------------------------------------

const _fileNodesStmtCache: StmtCache<{ id: number; file: string }> = new WeakMap();
const _allNodesStmtCache: StmtCache<{ id: number; file: string }> = new WeakMap();

export const FALSE_POSITIVE_NAMES = new Set([
'run',
'get',
Expand Down Expand Up @@ -45,10 +53,12 @@ export const FALSE_POSITIVE_CALLER_THRESHOLD = 20;
// ---------------------------------------------------------------------------

function buildTestFileIds(db: BetterSqlite3Database): Set<number> {
const allFileNodes = db.prepare("SELECT id, file FROM nodes WHERE kind = 'file'").all() as Array<{
id: number;
file: string;
}>;
const fileNodesStmt = cachedStmt(
_fileNodesStmtCache,
db,
"SELECT id, file FROM nodes WHERE kind = 'file'",
);
const allFileNodes = fileNodesStmt.all() as Array<{ id: number; file: string }>;
const testFileIds = new Set<number>();
const testFiles = new Set<string>();
for (const n of allFileNodes) {
Expand All @@ -57,10 +67,8 @@ function buildTestFileIds(db: BetterSqlite3Database): Set<number> {
testFiles.add(n.file);
}
}
const allNodes = db.prepare('SELECT id, file FROM nodes').all() as Array<{
id: number;
file: string;
}>;
const allNodesStmt = cachedStmt(_allNodesStmtCache, db, 'SELECT id, file FROM nodes');
Comment on lines 56 to +70
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Cache never hits in current call path

buildTestFileIds is a private helper called exclusively from statsData, which opens a fresh DB instance via openReadonlyOrFail(customDbPath) and closes it in the finally block on every invocation. Because each call to statsData produces a new BetterSqlite3Database object reference, the WeakMap key is always unique — the cache always misses and db.prepare() is called every time.

cachedStmt is only effective when the same db object is reused across multiple calls (e.g. a long-lived connection in MCP server mode). Since buildTestFileIds is not exported and statsData always opens-and-closes, the optimisation stated in the PR description ("reused across calls") does not materialise in practice.

The code is still functionally correct and consistent with the rest of the codebase's caching pattern — it is future-proof if statsData is ever refactored to accept an external db argument. But as written, both db.prepare() calls still execute on every invocation, so no performance gain is achieved.

Copy link
Copy Markdown
Contributor Author

@carlos-alm carlos-alm Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged — you're right that \ opens a fresh DB per call, so the cache doesn't hit in the current path. The change is intentionally forward-compatible: if \ or a future caller is refactored to accept a long-lived , the caching will activate automatically. Meanwhile it's a no-op with zero cost (WeakMap entry is immediately GC-eligible). Keeping for consistency with the rest of the codebase's \ pattern.

const allNodes = allNodesStmt.all() as Array<{ id: number; file: string }>;
for (const n of allNodes) {
if (testFiles.has(n.file)) testFileIds.add(n.id);
}
Expand Down
6 changes: 6 additions & 0 deletions src/domain/analysis/roles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ export function rolesData(
}
}

// NOTE: cachedStmt cannot be applied here because the SQL varies per call —
// the WHERE clause is built dynamically from `conditions` (role filter, file
// filter). A future optimisation could use a fixed SQL with CASE/COALESCE to
// absorb optional filters, or maintain a small Map<string, StmtCache> keyed
// by the unique condition combination (there are only ~4 variants). For now
// the dynamic prepare is acceptable given the low call frequency of `roles`.
let rows = db
.prepare(
`SELECT name, kind, file, line, end_line, role FROM nodes WHERE ${conditions.join(' AND ')} ORDER BY role, file, line`,
Expand Down
Loading