refactor: extract cachedStmt into reusable db utility#417
Conversation
Implement Phase 3.1 of the architectural refactoring roadmap. Replace 4 independent AST analysis passes (complexity, dataflow, AST-store, CFG) with a shared visitor framework that runs complexity, dataflow, and AST-store visitors in a single DFS walk per file. New framework files: - visitor.js: shared DFS walker with enterNode/exitNode/enterFunction/ exitFunction hooks, per-visitor skipChildren, nesting/scope tracking - engine.js: orchestrates all analyses in one coordinated pass, replaces 4 sequential buildXxx blocks + WASM pre-parse in builder.js - metrics.js: extracted Halstead derived math, LOC, MI from complexity.js - visitor-utils.js: extracted shared helpers from dataflow.js - visitors/complexity-visitor.js: cognitive/cyclomatic/nesting + Halstead - visitors/ast-store-visitor.js: new/throw/await/string/regex extraction - visitors/dataflow-visitor.js: scope stack + define-use chains - tests/unit/visitor.test.js: 9 tests for walker framework Modified modules: - builder.js: 4 sequential buildXxx blocks → single runAnalyses call - complexity.js: delegates walk to complexity visitor - dataflow.js: delegates walk to dataflow visitor - ast.js: delegates walk to ast-store visitor, removed dead code Bug fixes: - Fix Ruby dataflow parity: skip nested return keyword tokens inside return statements (tree-sitter Ruby grammar nests return/return) Hook fixes: - check-dead-exports.sh: exclude public API (index.js) and dynamic import() consumers from dead export detection - check-readme.sh: match ROADMAP.md at any path; allow commit when at least one doc is staged (developer reviewed which docs need updating) Docs: - ROADMAP.md: update 3.1 with completed items and remaining work - BACKLOG.md: add Tier 1h with dynamic import/re-export tracking (ID 71) - CLAUDE.md: add ast-analysis/ to architecture table Impact: 51 functions changed, 47 affected
The hook correctly catches a real limitation — codegraph doesn't track dynamic import() consumers as graph edges. The proper fix is to add dynamic import edge tracking (backlog ID 81), not to exclude them from the hook. Reverting the workaround.
…ex, remove unused var Impact: 2 functions changed, 23 affected
…ing, perf import - Replace halsteadSkip boolean with halsteadSkipDepth counter to handle nested skip-type nodes correctly (e.g. return_type containing type_annotation in TypeScript) - Add debug() logging to all silent catch blocks in engine.js so failures are discoverable with --verbose - Explicitly import performance from node:perf_hooks for clarity Impact: 5 functions changed, 24 affected
…ysis-framework Impact: 6 functions changed, 6 affected
…lookup Address two Greptile review comments on PR #388: 1. Function nodes in nestingNodeTypes inflates cognitive complexity by +1 for every function body — funcDepth already tracks function-level nesting, so adding functionNodes to the walker's nestingNodeTypes double-counts. 2. Replace O(n²) complexityResults.find() with O(1) resultByLine map lookup by storing the full result object (metrics + funcNode) instead of just metrics. Impact: 1 functions changed, 0 affected
…langId to LOC Address two Greptile review comments: 1. computeAllMetrics added functionNodes to nestingNodeTypes, inflating cognitive complexity by +1 for every function body (same bug as the engine.js fix in a47eb47, but in the per-function code path). 2. collectResult in complexity-visitor passed null as langId to computeLOCMetrics, causing C-style comment detection for all languages. Now accepts langId via options and forwards it, so Python/Ruby/PHP get correct comment-line counts and MI values. Impact: 4 functions changed, 5 affected
…ineOpts Address three Greptile review comments: 1. Wrap runAnalyses call in builder.js with try/catch + debug() so a walk-phase throw doesn't crash the entire build — matches the resilience of the previous individual buildXxx try/catch blocks. 2. In function-level mode, enterFunction/exitFunction were no-ops, so funcDepth stayed 0 for nested functions (callbacks, closures). Now increments/decrements funcDepth in function-level mode too, restoring parity with the original computeAllMetrics algorithm. 3. Rename _engineOpts to engineOpts in engine.js since it's actively forwarded to all four buildXxx delegate calls — the underscore prefix falsely signaled an unused parameter. Impact: 5 functions changed, 4 affected
…in hook
Address two Greptile review comments:
1. Remove the `processed` Set from dataflow-visitor.js — walkWithVisitors
visits each node exactly once via DFS on a proper tree, so the
deduplication guard never fires and just wastes memory.
2. Replace single-line destructuring regex in check-dead-exports.sh with
a multi-line-safe pattern (using the `s` flag) so multi-line
`const { ... } = await import(...)` patterns are correctly detected
and their names added to publicAPI.
Impact: 2 functions changed, 0 affected
Split src/db/repository.js into src/db/repository/ directory with 10 domain files: nodes, edges, build-stmts, complexity, cfg, dataflow, cochange, embeddings, graph-read, and barrel index. Extracted ~120 inline db.prepare() calls from 14 consumer modules into shared repository functions, eliminating duplication of common patterns like getNodeId (5 files), bulkNodeIdsByFile (3 files), callee/caller joins (6+ call sites), and file purge cascades. Net reduction: -437 lines across the codebase. All 1573 tests pass. Impact: 73 functions changed, 112 affected
…ed stmts, fix .raw() no-op - queries.js: restore DISTINCT-by-file deduplication for relatedTests in explainFunctionImpl (lost when switching to findCallers) - build-stmts.js: prepare SQL statements once in preparePurgeStmts() and loop with runPurge() instead of calling db.prepare() per file per table - cfg.js: replace misleading .raw() with .get() in hasCfgTables Impact: 7 functions changed, 14 affected
…ysis-framework Impact: 74 functions changed, 55 affected
db.prepare() was called on every iteration of the while loop in getClassHierarchy, causing repeated statement compilation for each ancestor level. Use a WeakMap cache (keyed by db instance) so the statement is compiled once and reused across iterations. Impact: 1 functions changed, 0 affected
Move the WeakMap-based prepared statement caching pattern into a reusable cachedStmt(cache, db, sql) helper in cached-stmt.js. Apply it to all 14 query functions in edges.js — each statement is now compiled once per db instance and reused on subsequent calls. Also hoists the cache lookup in getClassHierarchy before the BFS while loop so the WeakMap check runs once per call, not per iteration.
…caches Resolve add/add conflicts in cfg.js, edges.js, nodes.js, and index.js by using the cachedStmt utility from cached-stmt.js consistently across all repository modules. Preserves SELECT DISTINCT change from main in findCallees.
|
Claude finished @carlos-alm's task —— View job PR Review: Extract cachedStmt into Reusable DB UtilitySystematic review completed. Review Tasks:
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR addresses a performance regression where Original Bug (Pre-Fix): // BAD: db.prepare() called on every while loop iteration
while (queue.length > 0) {
const current = queue.shift();
const parents = db.prepare(`SELECT...`).all(current); // ❌ Repeated compilation
}Current Implementation: // GOOD: Statement cached once per db instance, hoisted before loop
const stmt = cachedStmt(_getClassAncestorsStmt, db, `SELECT...`);
while (queue.length > 0) {
const current = queue.shift();
const parents = stmt.all(current); // ✅ Reuse cached statement
}Why This Approach: Extracting the Phase 2: Configuration Change Detection ✅No High-Risk Changes Detected:
Phase 3: Test Integrity ✅Strong Test Coverage Maintained:
Phase 4: Code Quality Review ✅cachedStmt Utility Implementation:
Performance Benefits:
Security & Robustness:
Code Organization:
Minor Observations (not blocking):
Root Cause AnalysisProblem Being Solved: Performance bug in Critical ConcernsNone identified. This is a clean refactoring that fixes a real performance issue. Final Recommendation
Summary: This PR successfully addresses Greptile's feedback from #403 by extracting the caching pattern into a reusable utility. The |
Greptile SummaryThis PR extracts the recurring WeakMap-based prepared-statement caching pattern into a shared Key changes:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["caller\nedges.js / cfg.js / nodes.js"]
B["cachedStmt(cache, db, sql)"]
C{"cache.get(db)"}
D["return cached stmt"]
E["db.prepare(sql)"]
F["cache.set(db, stmt)"]
G["return new stmt"]
A --> B
B --> C
C -->|hit| D
C -->|miss| E
E --> F
F --> G
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _getCfgBlocksStmt, | ||
| db, | ||
| `SELECT id, block_index, block_type, start_line, end_line, label | ||
| FROM cfg_blocks WHERE function_node_id = ? | ||
| ORDER BY block_index`, | ||
| ).all(functionNodeId); | ||
| } | ||
|
|
||
| /** | ||
| * Get CFG edges for a function node (with block info). | ||
| * @param {object} db | ||
| * @param {number} functionNodeId | ||
| * @returns {object[]} | ||
| */ | ||
| export function getCfgEdges(db, functionNodeId) { | ||
| let stmt = _getCfgEdgesStmt.get(db); | ||
| if (!stmt) { | ||
| stmt = db.prepare( | ||
| `SELECT e.kind, | ||
| sb.block_index AS source_index, sb.block_type AS source_type, | ||
| tb.block_index AS target_index, tb.block_type AS target_type | ||
| FROM cfg_edges e | ||
| JOIN cfg_blocks sb ON e.source_block_id = sb.id | ||
| JOIN cfg_blocks tb ON e.target_block_id = tb.id | ||
| WHERE e.function_node_id = ? | ||
| ORDER BY sb.block_index, tb.block_index`, | ||
| ); | ||
| _getCfgEdgesStmt.set(db, stmt); | ||
| } | ||
| return stmt.all(functionNodeId); | ||
| return cachedStmt( | ||
| _getCfgEdgesStmt, | ||
| db, | ||
| `SELECT e.kind, | ||
| sb.block_index AS source_index, sb.block_type AS source_type, | ||
| tb.block_index AS target_index, tb.block_type AS target_type | ||
| FROM cfg_edges e | ||
| JOIN cfg_blocks sb ON e.source_block_id = sb.id | ||
| JOIN cfg_blocks tb ON e.target_block_id = tb.id | ||
| WHERE e.function_node_id = ? | ||
| ORDER BY sb.block_index, tb.block_index`, | ||
| ).all(functionNodeId); | ||
| } | ||
|
|
There was a problem hiding this comment.
New functions bypass the cachedStmt pattern they neighbour
getCfgBlocks, getCfgEdges, and deleteCfgForNode each call db.prepare() on every invocation. In src/cfg.js, both getCfgBlocks and getCfgEdges are called inside a nodes.map() loop (one per function node), and deleteCfgForNode is called inside the transaction loop in buildCFGData. For a codebase with many functions, this re-compiles four statements on every call.
The cachedStmt helper introduced in this very PR exists to avoid this. Consistent usage would be:
const _getCfgBlocksStmt = new WeakMap();
const _getCfgEdgesStmt = new WeakMap();
const _deleteCfgEdgesStmt = new WeakMap();
const _deleteCfgBlocksStmt = new WeakMap();
export function getCfgBlocks(db, functionNodeId) {
return cachedStmt(
_getCfgBlocksStmt,
db,
`SELECT id, block_index, block_type, start_line, end_line, label
FROM cfg_blocks WHERE function_node_id = ?
ORDER BY block_index`,
).all(functionNodeId);
}
// … same pattern for getCfgEdges, deleteCfgForNodehasCfgTables deliberately avoids cachedStmt (it catches a table-missing error), so that one is fine as-is.
There was a problem hiding this comment.
Already addressed in 139bde2 — all four functions now use cachedStmt with WeakMap caches. hasCfgTables deliberately avoids it since it catches table-missing errors.
|
Claude finished @carlos-alm's task —— View job PR Review: Extract cachedStmt into Reusable DB UtilitySystematic review completed. Review Tasks:
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR addresses a performance regression where Original Bug (Pre-Fix): // BAD: db.prepare() called on every while loop iteration
while (queue.length > 0) {
const current = queue.shift();
const parents = db.prepare(`SELECT...`).all(current); // ❌ Repeated compilation
}Current Implementation: // GOOD: Statement cached once per db instance, hoisted before loop
const stmt = cachedStmt(_getClassAncestorsStmt, db, `SELECT...`);
while (queue.length > 0) {
const current = queue.shift();
const parents = stmt.all(current); // ✅ Reuse cached statement
}Why This Approach: Extracting the Phase 2: Configuration Change Detection ✅No High-Risk Changes Detected:
Phase 3: Test Integrity ✅Strong Test Coverage Maintained:
Phase 4: Code Quality Review ✅cachedStmt Utility Implementation:
Performance Benefits:
Security & Robustness:
Code Organization:
Minor Observations (not blocking):
Root Cause AnalysisProblem Being Solved: Performance bug in Critical ConcernsNone identified. This is a clean refactoring that fixes a real performance issue. Final Recommendation
Summary: This PR successfully addresses Greptile's feedback from #403 by extracting the caching pattern into a reusable utility. The |
…tility # Conflicts: # src/db/repository/nodes.js Impact: 19 functions changed, 0 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Extract cachedStmt into Reusable DB UtilitySystematic review completed. Review Tasks:
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR addresses a performance regression where Original Bug (Pre-Fix): // BAD: db.prepare() called on every while loop iteration
while (queue.length > 0) {
const current = queue.shift();
const parents = db.prepare(`SELECT...`).all(current); // ❌ Repeated compilation
}Current Implementation: // GOOD: Statement cached once per db instance, hoisted before loop
const stmt = cachedStmt(_getClassAncestorsStmt, db, `SELECT...`);
while (queue.length > 0) {
const current = queue.shift();
const parents = stmt.all(current); // ✅ Reuse cached statement
}Why This Approach: Extracting the Phase 2: Configuration Change Detection ✅No High-Risk Changes Detected:
Phase 3: Test Integrity ✅Strong Test Coverage Maintained:
Phase 4: Code Quality Review ✅cachedStmt Utility Implementation:
Performance Benefits:
Security & Robustness:
Code Organization:
Minor Observations (not blocking):
Root Cause AnalysisProblem Being Solved: Performance bug in Critical ConcernsNone identified. This is a clean refactoring that fixes a real performance issue. Final Recommendation
Summary: This PR successfully addresses Greptile's feedback from #403 by extracting the caching pattern into a reusable utility. The |
1 participant
Summary
cachedStmt(cache, db, sql)helper insrc/db/repository/cached-stmt.jscachedStmtto all 14 query functions inedges.js— each statement is compiled once per db instance and reused on subsequent callsgetClassHierarchyBFS bug: hoists the cache lookup before thewhileloop so the WeakMap check runs once per call, not per iterationcachedStmtfrom the repository barrel (index.js) so other modules (e.g.sequence.js,queries.js) can adopt it incrementallyAddresses Greptile review feedback on #403: "The caching pattern could potentially be extracted into a reusable database utility."
Test plan