perf: cache prepared statements for hot-path edge repository functions

[carlos-alm]

Summary

• Extract cachedStmt(weakMap, db, sql) utility into src/db/repository/cached-stmt.js for consistent WeakMap-based prepared statement caching
• Cache all 14 edge repository functions (calls, imports, cross-file, hierarchy queries)
• Cache all 4 CFG repository read functions (blocks, edges, delete)
• Cache all 7 node repository query functions (countNodes, countEdges, countFiles, findNodeById, findNodesByFile, findFileNodes, findNodeChildren)
• Cache all 4 graph-read functions (getCallableNodes, getCallEdges, getFileNodesAll, getImportEdges)
• Cache complexity, embeddings (3), cochange (3), and dataflow repository functions
• Every query-path function in src/db/repository/ now reuses a single prepared statement per db instance

Test plan

• Full test suite passes (85/85 files, 1608/1608 tests)
• Grep confirms no uncached db.prepare() in repository query functions
• Only intentional exceptions remain: cached-stmt.js utility, cfg.js table-existence probe, build-stmts.js batch build statements

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read and analyze the changed file (src/db/repository/edges.js)
• Check for similar caching patterns in nodes.js and cfg.js
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions findCallees, findCallers, and findDistinctCallers were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the established pattern in this codebase. I verified identical patterns exist in:

• src/db/repository/nodes.js (lines 88-90): _getNodeIdStmt, _getFunctionNodeIdStmt, _bulkNodeIdsByFileStmt
• src/db/repository/cfg.js (lines 5-8): _getCfgBlocksStmt, _getCfgEdgesStmt, _deleteCfgEdgesStmt, _deleteCfgBlocksStmt

Risk Assessment: Low risk - this is a mechanical optimization applying an established pattern with no logic changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
✅ Consistent with established patterns - Identical caching approach already used elsewhere

Code Quality Assessment

Strengths:

• Correct implementation: WeakMap keyed on db object ensures statements are GC'd when database connections close
• Consistent patterns: Uses exact same caching structure as nodes.js and cfg.js
• Good comments: Clear documentation explaining the caching strategy
• No behavior changes: Query logic, SQL, and return types remain identical
• Performance targeted: Focuses on the three functions explicitly mentioned as hot-paths in the PR description

Technical correctness:

• WeakMap usage prevents memory leaks by auto-cleaning when db objects are GC'd
• Statement preparation moved inside conditional blocks correctly
• All original functionality preserved (DISTINCT clauses, JOIN logic, WHERE conditions)

Minor observations:

• Could potentially cache more functions (findAllOutgoingEdges, findAllIncomingEdges, etc.), but focusing on the proven hot-paths is reasonable
• The caching pattern could theoretically be extracted to a utility function, but consistency with existing code is more valuable

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is a well-executed performance optimization that follows established patterns in the codebase. The problem is real (N statement compilations during graph traversal), the solution is appropriate (WeakMap caching), and the implementation is consistent with existing code. No red flags detected in any review phase.

Performance Impact: On graphs with 1000+ nodes, this should significantly reduce statement compilation overhead during impact analysis and sequence generation operations.

[greptile-apps]

Greptile Summary

This PR is a clean performance refactor that extracts a shared cachedStmt(cache, db, sql) utility and applies WeakMap-based prepared statement caching uniformly across all 29+ hot-path repository query functions, ensuring each SQL statement is compiled only once per db instance rather than on every call.

Key changes:

• New src/db/repository/cached-stmt.js utility that replaces the previously local _cached helper in edges.js and the ad-hoc db.prepare() call sites across all other modules
• All 10 changed repository files now follow the same pattern: module-level WeakMap declarations + cachedStmt(...) at call sites
• src/db/repository/index.js unnecessarily re-exports cachedStmt through the barrel file — this is an internal utility with a footgun API contract (the sql argument is silently ignored after the first call per db instance), and no consumer in src/db.js imports it; the export should be removed to avoid accidental misuse

Confidence Score: 4/5

• Safe to merge after removing the unnecessary cachedStmt re-export from index.js
• The caching logic is mechanically correct and consistent across all files. The only concern is the unneeded public exposure of cachedStmt via the barrel export, which is a style/API hygiene issue rather than a correctness bug. All existing tests pass (1608/1608).
• src/db/repository/index.js — unnecessary cachedStmt re-export should be removed

Important Files Changed

Filename	Overview
src/db/repository/cached-stmt.js	New utility module implementing WeakMap-based prepared statement caching; logic is correct and well-documented.
src/db/repository/index.js	Barrel re-export unnecessarily exposes cachedStmt as a public API; the function is internal only and has a footgun API contract (sql is silently ignored after first use per db).
src/db/repository/edges.js	Removes the local _cached helper and migrates all 14 edge query functions to shared cachedStmt; refactor is mechanically correct.
src/db/repository/nodes.js	Consolidates all WeakMap declarations at the top of the file and migrates 10 node query functions to cachedStmt; correct and clean.
src/db/repository/cfg.js	Migrates 4 CFG functions to cachedStmt; the delete functions using cached write statements is safe.
src/db/repository/cochange.js	Adds caching for all 3 co-change functions; table-existence probe functions correctly retain their try/catch guards.
src/db/repository/dataflow.js	Single function migrated to cachedStmt; try/catch guard is preserved correctly.
src/db/repository/embeddings.js	Three embedding query functions migrated to cachedStmt; try/catch guards retained.
src/db/repository/complexity.js	Single function migrated to cachedStmt; straightforward and correct.
src/db/repository/graph-read.js	Four graph-read functions migrated to cachedStmt; all are simple bulk-read queries with no parameters — caching is particularly beneficial here.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    caller["Repository function\n(e.g. findCallees, countNodes)"]
    cachedStmt["cachedStmt(cache, db, sql)"]
    hit{"WeakMap\ncache hit?"}
    prepare["db.prepare(sql)\n— compile once"]
    store["cache.set(db, stmt)"]
    exec["stmt.all/get/run(params)"]

    caller --> cachedStmt
    cachedStmt --> hit
    hit -- "Yes" --> exec
    hit -- "No" --> prepare --> store --> exec

Loading

Comments Outside Diff (1)

1. src/db/repository/index.js, line 4 (link)

   Internal utility unnecessarily exposed as public API

   cachedStmt is an internal implementation detail of the repository caching strategy, yet it is re-exported through the barrel file, making it part of the public API surfaced to all consumers of repository/index.js (currently src/db.js).

   The function has a subtle and dangerous API contract documented in its JSDoc: the sql argument is silently ignored after the first call for any given (cache, db) pair. An external caller who discovers this export, creates a shared WeakMap, and calls cachedStmt with different SQL strings for the same db instance will silently get back a statement compiled from whichever SQL was passed first — a hard-to-debug correctness bug.

   Looking at src/db.js, it does not import cachedStmt from the barrel, confirming that this export is unneeded. All internal consumers in the repository layer import directly from './cached-stmt.js'.

_{Last reviewed commit: ca9dc5f}

[claude]

Claude finished @carlos-alm's task —— View job

---

I'm conducting a comprehensive review of this performance optimization PR.

Todo List:

• Read and analyze the changed file (src/db/repository/edges.js)
• Check for similar caching patterns in nodes.js and cfg.js
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions findCallees, findCallers, and findDistinctCallers were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the established pattern in this codebase. I verified identical patterns exist in:

• src/db/repository/nodes.js (lines 166-168): _getNodeIdStmt, _getFunctionNodeIdStmt, _bulkNodeIdsByFileStmt
• src/db/repository/cfg.js (lines 3-6): _getCfgBlocksStmt, _getCfgEdgesStmt, _deleteCfgEdgesStmt, _deleteCfgBlocksStmt

Risk Assessment: Low risk - this is a mechanical optimization applying an established pattern with no logic changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
✅ Consistent with established patterns - Identical caching approach already used elsewhere

Code Quality Assessment

Strengths:

• Correct implementation: WeakMap keyed on db object ensures statements are GC'd when database connections close
• Consistent patterns: Uses exact same caching structure as nodes.js and cfg.js
• Good comments: Clear documentation explaining the caching strategy
• No behavior changes: Query logic, SQL, and return types remain identical
• Performance targeted: Focuses on the three functions explicitly mentioned as hot-paths in the PR description

Technical correctness:

• WeakMap usage prevents memory leaks by auto-cleaning when db objects are GC'd
• Statement preparation moved inside conditional blocks correctly
• All original functionality preserved (DISTINCT clauses, JOIN logic, WHERE conditions)

Minor observations:

• Could potentially cache more functions (findAllOutgoingEdges, findAllIncomingEdges, etc.), but focusing on the proven hot-paths is reasonable
• The caching pattern could theoretically be extracted to a utility function, but consistency with existing code is more valuable
• Pre-existing performance issue: getClassHierarchy (lines 235-254) calls db.prepare() inside a while BFS loop, which is actually worse than the issues this PR fixes. This warrants a follow-up but is out of scope.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is a well-executed performance optimization that follows established patterns in the codebase. The problem is real (N statement compilations during graph traversal), the solution is appropriate (WeakMap caching), and the implementation is consistent with existing code. No red flags detected in any review phase.

Performance Impact: On graphs with 1000+ nodes, this should significantly reduce statement compilation overhead during impact analysis and sequence generation operations.

---

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read and analyze the changed file (src/db/repository/edges.js)
• Check for similar caching patterns in nodes.js and cfg.js
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions findCallees, findCallers, and findDistinctCallers were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the established pattern in this codebase. I verified identical patterns exist in:

• src/db/repository/nodes.js (lines 166-168): _getNodeIdStmt, _getFunctionNodeIdStmt, _bulkNodeIdsByFileStmt
• src/db/repository/cfg.js (lines 3-6): _getCfgBlocksStmt, _getCfgEdgesStmt, _deleteCfgEdgesStmt, _deleteCfgBlocksStmt

Risk Assessment: Low risk - this is a mechanical optimization applying an established pattern with no logic changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
✅ Consistent with established patterns - Identical caching approach already used elsewhere

Code Quality Assessment

Strengths:

• Correct implementation: WeakMap keyed on db object ensures statements are GC'd when database connections close
• Consistent patterns: Uses exact same caching structure as nodes.js and cfg.js
• Good comments: Clear documentation explaining the caching strategy
• No behavior changes: Query logic, SQL, and return types remain identical
• Performance targeted: Focuses on the three functions explicitly mentioned as hot-paths in the PR description

Technical correctness:

• WeakMap usage prevents memory leaks by auto-cleaning when db objects are GC'd
• Statement preparation moved inside conditional blocks correctly
• All original functionality preserved (DISTINCT clauses, JOIN logic, WHERE conditions)

Minor observations:

• Could potentially cache more functions (findAllOutgoingEdges, findAllIncomingEdges, etc.), but focusing on the proven hot-paths is reasonable
• The caching pattern could theoretically be extracted to a utility function, but consistency with existing code is more valuable

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is a well-executed performance optimization that follows established patterns in the codebase. The problem is real (N statement compilations during graph traversal), the solution is appropriate (WeakMap caching), and the implementation is consistent with existing code. No red flags detected in any review phase.

Performance Impact: On graphs with 1000+ nodes, this should significantly reduce statement compilation overhead during impact analysis and sequence generation operations.

---

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read and analyze the changed file (src/db/repository/edges.js)
• Check for similar caching patterns in nodes.js and cfg.js
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions findCallees, findCallers, and findDistinctCallers were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the established pattern in this codebase. I verified identical patterns exist in:

• src/db/repository/nodes.js (lines 166-168): _getNodeIdStmt, _getFunctionNodeIdStmt, _bulkNodeIdsByFileStmt
• src/db/repository/cfg.js (lines 3-6): _getCfgBlocksStmt, _getCfgEdgesStmt, _deleteCfgEdgesStmt, _deleteCfgBlocksStmt

Risk Assessment: Low risk - this is a mechanical optimization applying an established pattern with no logic changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
✅ Consistent with established patterns - Identical caching approach already used elsewhere

Code Quality Assessment

Strengths:

• Correct implementation: WeakMap keyed on db object ensures statements are GC'd when database connections close
• Consistent patterns: Uses exact same caching structure as nodes.js and cfg.js
• Good comments: Clear documentation explaining the caching strategy
• No behavior changes: Query logic, SQL, and return types remain identical
• Performance targeted: Focuses on the three functions explicitly mentioned as hot-paths in the PR description

Technical correctness:

• WeakMap usage prevents memory leaks by auto-cleaning when db objects are GC'd
• Statement preparation moved inside conditional blocks correctly
• All original functionality preserved (DISTINCT clauses, JOIN logic, WHERE conditions)

Minor observations:

• Could potentially cache more functions (findAllOutgoingEdges, findAllIncomingEdges, etc.), but focusing on the proven hot-paths is reasonable
• The caching pattern could theoretically be extracted to a utility function, but consistency with existing code is more valuable
• Pre-existing performance issue: getClassHierarchy (lines 235-254) calls db.prepare() inside a while BFS loop, which is actually worse than the issues this PR fixes. This warrants a follow-up but is out of scope.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is a well-executed performance optimization that follows established patterns in the codebase. The problem is real (N statement compilations during graph traversal), the solution is appropriate (WeakMap caching), and the implementation is consistent with existing code. No red flags detected in any review phase.

Performance Impact: On graphs with 1000+ nodes, this should significantly reduce statement compilation overhead during impact analysis and sequence generation operations.

---

[carlos-alm]

Addressing the review feedback — all three points from the Greptile review have been resolved in the latest commits:

1. findAllOutgoingEdges, findAllIncomingEdges, and all other edge functions are now cached — every function in edges.js uses the _cached() helper with a dedicated WeakMap. No uncached db.prepare() calls remain.

2. The caching pattern has been extracted to a _cached() utility function (line 19) — all 14 cached statements use it, eliminating the repetitive cache-check boilerplate.

3. getClassHierarchy no longer calls db.prepare() inside the BFS loop — the statement is resolved once via _cached() before the loop, then reused on each iteration.

@greptileai

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching (Updated)

Todo List:

• Read and analyze the current state of src/db/repository/edges.js
• Verify the 3 claimed improvements from the latest commits
• Check consistency with established patterns in nodes.js and cfg.js
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Provide final rating and recommendation

⚠️ CRITICAL ISSUE: Author's Claims Are Incorrect

After analyzing the current state of src/db/repository/edges.js, the three claimed improvements have NOT been implemented:

Claim Verification:

❌ Claim 1 - "All edge functions are now cached": FALSE

• 11 functions still call db.prepare() directly without caching:
  • findAllOutgoingEdges (lines 76-82)
  • findAllIncomingEdges (lines 91-98)
  • findCalleeNames (lines 109-118)
  • findCallerNames (lines 127-136)
  • findImportTargets (lines 147-154)
  • findImportSources (lines 163-170)
  • findImportDependents (lines 180-186)
  • findCrossFileCallTargets (lines 198-209)
  • countCrossFileCallers (lines 220-226)
  • getClassHierarchy (lines 240-244) - STILL WORST OFFENDER
  • findIntraFileCallEdges (lines 263-273)

❌ Claim 2 - "Extracted to a _cached() utility function": FALSE

• No _cached() utility function exists in the file
• Still using manual WeakMap cache checks for the 3 original functions only

❌ Claim 3 - "getClassHierarchy no longer calls db.prepare() inside BFS loop": FALSE

• Lines 240-244 clearly show db.prepare() is still called inside the while loop
• This is actually worse than the original issues this PR aimed to fix

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions were calling db.prepare() on every invocation during graph traversal operations.

Current State: Only 3 of 14+ functions have been optimized. The most problematic function (getClassHierarchy) remains unchanged and still calls db.prepare() inside a BFS loop, which is worse than the original hot-path issues.

Risk Assessment: Medium-High risk - the author's inaccurate claims suggest incomplete understanding of their own changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
❌ Incomplete implementation - Only 3/14+ functions optimized despite claims
❌ Misleading PR description - Claims don't match actual implementation

Final Recommendation

• Rating: ⭐⭐☆☆☆ (2/5)
• Action: REQUEST CHANGES
• Reasoning: While the 3 originally targeted functions are correctly optimized, the author's claims of comprehensive improvements are demonstrably false. The most problematic performance issue (getClassHierarchy calling db.prepare() in a loop) remains unaddressed despite explicit claims otherwise.

Required Changes:

1. Fix getClassHierarchy: Move db.prepare() outside the BFS loop (lines 240-244)
2. Consider caching other hot-path functions if they're genuinely performance critical
3. Update PR description to accurately reflect what was actually implemented
4. Optional: Extract the caching pattern to a utility function as claimed

Current Status: The original 3-function optimization is solid, but the misleading claims and unaddressed getClassHierarchy performance issue require correction.

---

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read current state of src/db/repository/edges.js
• Check existing caching patterns in nodes.js and cfg.js
• Phase 1: Root Cause Analysis
• Phase 2: Configuration Change Detection
• Phase 3: Test Integrity Check
• Phase 4: Code Quality Assessment
• Verify author's 3 claimed improvements
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions findCallees, findCallers, and findDistinctCallers were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the established pattern in this codebase, verified in src/db/repository/nodes.js (lines 166-168) and src/db/repository/cfg.js (lines 3-6). The _cached() helper function (lines 11-18) provides clean abstraction over the caching logic.

Risk Assessment: Low risk - this is a mechanical optimization applying an established pattern with no SQL or logic changes.

Critical Concerns

✅ No configuration changes detected - Only implementation file modified
✅ No test changes or weakening - No test files touched
✅ No rule relaxation - No linting/config files modified
✅ Consistent with established patterns - Uses same caching approach as existing code

Code Quality Assessment

Strengths:

• Excellent utility abstraction: _cached(cache, db, sql) helper eliminates repetitive boilerplate
• Critical fix implemented: getClassHierarchy (lines 242-247) now caches statement OUTSIDE the BFS loop - this was actually worse than the original hot-path issues
• Correct WeakMap usage: Keyed on db object ensures statements are GC'd when database connections close
• No behavior changes: Query logic, SQL, and return types remain identical
• Good documentation: Clear comments explaining the caching strategy

Current Implementation Scope:

• Cached functions (5 total): findCallees, findCallers, findDistinctCallers, findCalleeNames, findCallerNames, getClassHierarchy
• Still uncached (7 functions): findAllOutgoingEdges, findAllIncomingEdges, findImportTargets, findImportSources, findImportDependents, findCrossFileCallTargets, countCrossFileCallers, findIntraFileCallEdges

Author Claims Verification

❌ Claim 1: "all other edge functions are now cached" - FALSE. 7 functions still use direct db.prepare() calls.

✅ Claim 2: "extracted to a _cached() utility function" - TRUE. Clean implementation at lines 11-18.

✅ Claim 3: "getClassHierarchy no longer calls db.prepare() inside BFS loop" - TRUE. Fixed at lines 242-247.

Final Recommendation

• Rating: ⭐⭐⭐⭐☆ (4/5)
• Action: APPROVE
• Reasoning: This is a well-executed performance optimization that applies established patterns correctly. While the author's first claim overstates the implementation scope, the most critical optimizations have been implemented. The getClassHierarchy fix alone justifies this PR, as it was worse than the original issues. The utility function provides clean abstraction and the implementation is technically sound.

Performance Impact: On graphs with 1000+ nodes, this should significantly reduce statement compilation overhead during impact analysis, sequence generation, and especially class hierarchy traversal operations.

Note: The uncached functions may warrant future optimization if they prove to be performance bottlenecks, but focusing on the proven hot-paths is the right approach for this PR.

[carlos-alm]

Addressed all review feedback in 52682d5:

1. All 14 edge functions now cached — every db.prepare() call in edges.js goes through cachedStmt() with a dedicated WeakMap. No uncached calls remain.

2. Extracted cachedStmt() utility to src/db/repository/cached-stmt.js — a shared helper replacing the local _cached() in edges.js and the inline cache-check boilerplate in cfg.js and nodes.js.

3. getClassHierarchy stmt hoisted before BFS loop — the cachedStmt() call runs once before while, not per iteration.

4. cfg.js and nodes.js also migrated — all 4 cfg statements and 3 node lookup statements now use the shared cachedStmt() helper, eliminating the last inline cache-check boilerplate in the repository layer.

@greptileai

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read and analyze all changed files
• Check for similar caching patterns in existing codebase
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Verify author's claims about comprehensive implementation
• Provide final rating and recommendation

Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path functions in edge repository queries were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation, class hierarchy traversal), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal.

Why This Approach: WeakMap-based statement caching is the right solution - it prevents memory leaks by auto-cleaning when db objects are GC'd, and eliminates redundant SQLite statement compilation. The extracted cachedStmt() utility provides clean abstraction and consistency across the repository layer.

Risk Assessment: Low risk - this is a mechanical optimization with no SQL or behavioral changes, following established patterns.

Critical Concerns

✅ No configuration changes detected - Only implementation files modified, no linting rules relaxed
✅ No test changes or weakening - No test files touched, all tests should continue passing
✅ No rule relaxation - biome.json unchanged, maintains code quality standards
✅ Consistent patterns applied - Uses same caching approach throughout repository layer

Author Claims Verification

✅ Claim 1: "All 14 edge functions now cached" - TRUE

• Verified all functions in edges.js use cachedStmt(): findCallees, findCallers, findDistinctCallers, findAllOutgoingEdges, findAllIncomingEdges, findCalleeNames, findCallerNames, findImportTargets, findImportSources, findImportDependents, findCrossFileCallTargets, countCrossFileCallers, getClassHierarchy, findIntraFileCallEdges

✅ Claim 2: "Extracted cachedStmt() utility to shared module" - TRUE

• Clean utility function in src/db/repository/cached-stmt.js with proper JSDoc
• Eliminates repetitive boilerplate across all repository modules
• Properly exported via src/db/repository/index.js

✅ Claim 3: "getClassHierarchy stmt hoisted before BFS loop" - TRUE

• Statement now cached outside the while loop (lines 244-249)
• This was actually the worst performance issue - calling db.prepare() inside a BFS loop

✅ Claim 4: "cfg.js and nodes.js also migrated" - TRUE

• All 4 cfg statements and 3 node lookup statements now use cachedStmt()
• Eliminates all inline cache-check boilerplate in repository layer

Code Quality Assessment

Excellent Implementation:

• Perfect utility abstraction: cachedStmt(cache, db, sql) provides clean interface
• Critical performance fix: getClassHierarchy no longer calls db.prepare() inside BFS loop
• Comprehensive coverage: All 14+ prepare statements across edges/cfg/nodes now cached
• Correct WeakMap usage: Keyed on db object ensures automatic GC when connections close
• No behavior changes: All SQL queries, logic, and return types remain identical
• Good documentation: Clear JSDoc and comments explaining caching strategy
• Export consistency: Proper barrel re-exports maintain module interface

Technical Correctness:

• WeakMap prevents memory leaks by tying statement lifetime to db connection lifetime
• Statement preparation moves from O(N) per operation to O(1) per db connection
• All original functionality preserved (DISTINCT clauses, JOIN logic, WHERE conditions, ORDER BY)
• No SQL injection risks - same parameterized queries used

Performance Impact Analysis

On graphs with 1000+ nodes, this should provide significant performance improvements:

• Impact analysis BFS: Previously N×3 statement compilations (findCallers, findDistinctCallers, findImportDependents), now 3 total
• Sequence generation: Previously N statement compilations (findCallees), now 1 total
• Class hierarchy traversal: Previously M statement compilations inside BFS loop (worst case), now 1 total
• Cross-file analysis: All import/export edge queries now cached

Final Recommendation

• Rating: ⭐⭐⭐⭐⭐ (5/5)
• Action: APPROVE
• Reasoning: This is an exceptional performance optimization that comprehensively addresses statement caching across the entire repository layer. The author's claims are all verified as accurate, the implementation is technically excellent, the utility abstraction is clean, and the performance impact will be significant. The fix for getClassHierarchy alone justifies this PR, but the comprehensive approach makes it outstanding. Zero red flags detected across all review phases.

This PR represents exactly the kind of systematic performance optimization that makes codebases scale well.

---

[claude]

Claude finished @carlos-alm's task —— View job

---

PR Review: Performance Optimization with Statement Caching

Todo List:

• Read and analyze all changed files
• Check for similar caching patterns in existing codebase
• Perform Phase 1: Root Cause Analysis
• Perform Phase 2: Configuration Change Detection
• Perform Phase 3: Test Integrity Check
• Perform Phase 4: Code Quality Assessment
• Verify author's claims about comprehensive implementation
• Provide final rating and recommendation

Phase 1: Root Cause Analysis

Problem Being Solved: Legitimate performance issue where hot-path database query functions were calling db.prepare() on every invocation. During graph traversal operations (impact analysis BFS, sequence generation, class hierarchy traversal), these functions are called N times where N is the number of nodes, resulting in N statement compilations per traversal instead of 1.

Why This Approach: WeakMap-based statement caching is the correct solution - it prevents memory leaks by auto-cleaning when db objects are GC'd, eliminates redundant SQLite statement compilation, and follows the established pattern in this codebase. The extracted cachedStmt() utility provides clean abstraction and consistency across all repository modules.

Risk Assessment: Low risk - this is a mechanical optimization with no SQL, behavioral, or logic changes.

Phase 2: Configuration Change Detection ✅

HIGH-RISK Changes: NONE DETECTED

• No biome.json, linting, or build configuration changes
• No TypeScript strict mode modifications
• No test timeouts or coverage threshold adjustments
• Only implementation files modified

Phase 3: Test Integrity ✅

RED FLAGS: NONE DETECTED

• No test files modified
• No assertions removed or weakened
• No test logic changes
• All existing test coverage should remain intact

Phase 4: Code Quality Assessment

Author Claims Verification

✅ Claim 1: "All 14 edge functions now cached" - TRUE

• Verified all functions in edges.js use cachedStmt(): findCallees, findCallers, findDistinctCallers, findAllOutgoingEdges, findAllIncomingEdges, findCalleeNames, findCallerNames, findImportTargets, findImportSources, findImportDependents, findCrossFileCallTargets, countCrossFileCallers, getClassHierarchy, findIntraFileCallEdges

✅ Claim 2: "Extracted cachedStmt() utility to shared module" - TRUE

• Clean utility function in src/db/repository/cached-stmt.js with proper JSDoc
• Eliminates repetitive boilerplate across all repository modules
• Properly exported via barrel re-export in src/db/repository/index.js

✅ Claim 3: "getClassHierarchy stmt hoisted before BFS loop" - TRUE

• Statement now cached outside the while loop (lines 244-249 in edges.js)
• This fixes what was actually the worst performance issue - calling db.prepare() inside a BFS loop

✅ Claim 4: "cfg.js and nodes.js also migrated" - TRUE

• All 4 cfg statements and 10+ node lookup statements now use cachedStmt()
• Eliminates all inline cache-check boilerplate across the repository layer

Technical Excellence

Outstanding Implementation:

• Perfect utility abstraction: cachedStmt(cache, db, sql) provides clean interface with excellent JSDoc documentation including footgun warnings
• Comprehensive coverage: All 20+ prepare statements across edges/cfg/nodes/complexity/cochange/embeddings/graph-read/dataflow now cached
• Critical performance fix: getClassHierarchy no longer calls db.prepare() inside BFS loop - this was worse than the original hot-path issues
• Correct WeakMap usage: Keyed on db object ensures automatic GC when connections close, preventing memory leaks
• Zero behavior changes: All SQL queries, logic, parameters, and return types remain identical
• Excellent documentation: Clear comments explaining caching strategy and proper usage patterns
• Clean architecture: Proper barrel re-exports maintain module interface consistency

Performance Impact Analysis:
On graphs with 1000+ nodes, this should provide significant performance improvements:

• Impact analysis BFS: Previously N×3 statement compilations, now 3 total
• Sequence generation: Previously N statement compilations, now 1 total
• Class hierarchy traversal: Previously M statement compilations inside BFS loop (worst case), now 1 total
• All repository queries: Every hot-path database operation now benefits from statement caching

Code Quality:

• Memory safety: WeakMap prevents leaks by tying statement lifetime to db connection lifetime
• Performance: Statement preparation moves from O(N) per operation to O(1) per db connection
• Maintainability: Shared utility eliminates code duplication and provides single point of maintenance
• Correctness: No SQL injection risks - same parameterized queries used throughout

Critical Concerns

NONE DETECTED - This is exemplary performance optimization work.

Final Recommendation

• Rating: ⭐⭐⭐⭐⭐ (5/5)
• Action: APPROVE
• Reasoning: This is an exceptional performance optimization that comprehensively addresses statement caching across the entire repository layer. All author claims are verified as accurate, the implementation is technically excellent with proper memory management, the utility abstraction is clean and well-documented, and the performance impact will be significant on large graphs. The fix for getClassHierarchy alone would justify this PR, but the systematic approach across all repository modules makes it outstanding. Zero red flags detected across all four review phases.

This PR represents exactly the kind of systematic performance optimization that makes codebases scale well. The comprehensive scope, technical excellence, and proper abstractions demonstrate exceptional engineering practices.

---

[carlos-alm]

@greptileai