Skip to content

perf(native): fix WASM fallback bypass and batch SQL inserts#606

Merged
carlos-alm merged 8 commits intomainfrom
perf/native-bypass-and-batch-inserts
Mar 26, 2026
Merged

perf(native): fix WASM fallback bypass and batch SQL inserts#606
carlos-alm merged 8 commits intomainfrom
perf/native-bypass-and-batch-inserts

Conversation

@carlos-alm
Copy link
Contributor

Summary

  • Fix WASM fallback bug: Interface property signatures (dotted names like Interface.prop, single-line spans) were incorrectly triggering WASM tree creation on native builds. Fixed in engine.ts (ensureWasmTreesIfNeeded, setupVisitors), complexity.ts (initWasmParsersIfNeeded), and cfg.ts (initCfgParsers) by adding !d.name.includes('.') and endLine > line filters.
  • Batch SQL insert optimizations: Statement caching via WeakMap in helpers.ts, chunk size 200→500, batched export marking in insert-nodes.ts, batch UPDATE nodes SET role = ? WHERE id IN (...) grouped by role in structure.ts.
  • Lint cleanup: Replace ! non-null assertions with safe alternatives in touched files.

Benchmark results (native vs WASM, full build on codegraph itself)

Phase WASM Native Speedup
parseMs 1212 496 2.4x
insertMs 346 321 1.1x
complexityMs 209 50 4.2x
cfgMs 202 64 3.2x
astMs 432 340 1.3x
finalizeMs 177 112 1.6x
Total build 3116 2001 1.6x

Test plan

  • All 2106 tests pass, 0 failures
  • Benchmark confirms native faster than WASM on all key phases
  • Verified dataflow.ts does NOT have the same bug (native always provides symbols.dataflow for supported languages)
  • Verify no regression on WASM-only builds (no native addon)

Add `npm run benchmark` script to make benchmark execution discoverable
instead of requiring manual `node --import ./scripts/ts-resolve-loader.js`
invocation.

Warn users when embeddings predate the last graph rebuild so they know
to re-run `codegraph embed` for fresh search results.

Impact: 1 functions changed, 8 affected
Fix interface property signatures (dotted names, single-line spans)
incorrectly triggering WASM tree creation on native builds across
engine.ts, complexity.ts, and cfg.ts. Add statement caching and
batch UPDATE optimizations for insert and role classification stages.

Native full build: 2001ms vs WASM 3116ms (1.6x faster).
Key wins: complexity 4.2x, cfg 3.2x, parse 2.4x faster.

Impact: 26 functions changed, 25 affected
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR delivers two independent improvements: a correctness fix that prevents interface/type property signatures from incorrectly triggering the WASM parser fallback on native builds, and a suite of SQL batch-insert optimizations that reduce per-iteration statement compilation and statement count. Together they yield a measured 1.6× end-to-end speedup on a full native build.

Key changes:

  • WASM fallback bypass fix (engine.ts, cfg.ts, complexity.ts): A new hasFuncBody predicate filters definitions to only those with endLine > line (real body span) and !name.includes('.') (not a dotted interface-property signature). Previously, single-line type property signatures such as Interface.prop were extracted as method nodes with no complexity/CFG data, causing the engine to think native output was incomplete and fall back to WASM unnecessarily.
  • Statement caching (helpers.ts): WeakMap<Database, Map<chunkSize, Statement>> caches compiled INSERT statements per database instance per chunk size, avoiding re-compilation on every batch iteration. Chunk size also raised from 200 → 500.
  • Merged insert phases (insert-nodes.ts): The former three-phase insertChildren + insertContainmentEdges pipeline is collapsed into insertChildrenAndEdges, which makes two passes over allSymbols but only one bulkNodeIdsByFile DB call per file per pass (unchanged round-trip count, cleaner code). Export-marking UPDATEs are now batched with OR conditions (up to 500 per statement) instead of one per export.
  • Batched role assignment (structure.ts): classifyNodeRoles now groups node IDs by role and issues one UPDATE nodes SET role = ? WHERE id IN (...) per chunk instead of one UPDATE per node.
  • Lint cleanup: ! non-null assertions replaced with ?. / ?? throughout touched files; verified safe at each call site (e.g., upsertPrecomputedComplexity is only reached when def.complexity is truthy, tree?.rootNode is only reached after a !needsVisitor guard that checks tree is non-null).

Confidence Score: 5/5

  • Safe to merge — correctness fix is well-targeted, all 2106 tests pass, and the batch optimizations are structurally sound.
  • The WASM fallback fix is narrowly scoped and applied consistently across all three sites (engine.ts, cfg.ts, complexity.ts). The batch SQL optimizations follow the same pattern as the existing WeakMap cache and don't change observable semantics. Every !?. change was verified to be safe at its specific call site. Previous reviewer concerns about optional-chaining widening types and the export-stmt cache were both addressed. No new logic paths are uncovered by the 2106-test suite.
  • No files require special attention.

Important Files Changed

Filename Overview
src/ast-analysis/engine.ts Introduces hasFuncBody helper to filter out interface/type property signatures (dotted names, single-line span) so they no longer trigger WASM tree creation on native builds. Logic is consistent with the parallel fixes in cfg.ts and complexity.ts.
src/domain/graph/builder/helpers.ts Adds WeakMap-keyed statement caches (nodeStmtCache, edgeStmtCache) so prepared INSERT statements are compiled once per chunk size per database instance. Chunk size raised from 200 to 500. Clean and well-structured optimization.
src/domain/graph/builder/stages/insert-nodes.ts Merges the old insertChildren + insertContainmentEdges phases into a single insertChildrenAndEdges that uses two passes: first collecting file→def edges and child rows, then (after inserting children) re-fetching IDs for def→child edges. Export marking is now batched with OR conditions up to 500 per UPDATE instead of one UPDATE per export.
src/domain/graph/builder/stages/finalize.ts Adds a stale-embeddings warning that compares the embedding built_at timestamp against the last graph rebuild timestamp. Wrapped in try/catch since the embedding_meta table may not exist. Straightforward feature addition.
src/features/cfg.ts Applies the same multi-line/non-dotted-name filter as engine.ts to hasNativeCfgForFile, so single-line interface property signatures no longer cause the CFG phase to think native data is missing. Also replaces tree!.rootNode with tree?.rootNode — safe because the !needsVisitor guard above ensures tree is non-null before that line.
src/features/complexity.ts Adds the same endLine/dotted-name guards to initWasmParsersIfNeeded. Replaces rules!.xxx with rules?.xxx throughout computeFunctionComplexity/computeHalsteadMetrics — safe because callers always pass a valid rules object; ?. just avoids TypeScript non-null assertions without changing runtime behavior in the guarded call paths.
src/features/structure.ts Batches role UPDATE into one statement per role per chunk (IN clause), replacing the original per-node loop. Groups nodes by role into idsByRole map, caches statements by chunk size, and runs a single UPDATE nodes SET role = ? WHERE id IN (...) per chunk. Lint fixes replace ! with ?? for density calculations and null-safe Map access.

Sequence Diagram

sequenceDiagram
    participant T as insertNodes (transaction)
    participant P1 as insertDefinitionsAndExports
    participant P23 as insertChildrenAndEdges
    participant DB as SQLite DB

    T->>P1: call
    P1->>DB: batchInsertNodes(files + defs + exports)<br/>INSERT OR IGNORE INTO nodes [chunks of 500]
    P1->>DB: batch UPDATE exported=1<br/>WHERE OR conditions [chunks of 500]
    P1-->>T: done

    T->>P23: call
    Note over P23: Pass 1 – collect file→def edges & child rows
    P23->>DB: bulkNodeIdsByFile (per file)
    P23-->>P23: accumulate edgeRows[file→def] + childRows
    P23->>DB: batchInsertNodes(childRows)<br/>INSERT OR IGNORE INTO nodes [chunks of 500]

    Note over P23: Pass 2 – re-fetch IDs (now includes children)
    P23->>DB: bulkNodeIdsByFile (per file, again)
    P23-->>P23: accumulate edgeRows[def→child + parameter_of]
    P23->>DB: batchInsertEdges(all edgeRows)<br/>INSERT INTO edges [chunks of 500]
    P23-->>T: done
Loading

Reviews (3): Last reviewed commit: "fix: correct misleading comments and cac..." | Re-trigger Greptile

Comment on lines +746 to 749
const stmt = HOTSPOT_QUERIES[metric] ?? HOTSPOT_QUERIES['fan-in'];
const rows = stmt?.all(kind, limit);

const hotspots = rows.map((r) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Unnecessary optional chaining produces potentially-undefined rows

HOTSPOT_QUERIES['fan-in'] is always a concrete Statement (it's a required property of the local object literal), so after the ?? fallback stmt is non-nullable. The ?. on stmt.all() is therefore superfluous, but depending on how TypeScript resolves the type, it may widen rows to Statement.all(...) | undefined. The immediately following rows.map(...) on line 749 has no null guard, so if TypeScript infers rows as T[] | undefined this is a latent type error. Either keep the original ! assertions (which were semantically accurate), or add an explicit null check before the map:

Suggested change
const stmt = HOTSPOT_QUERIES[metric] ?? HOTSPOT_QUERIES['fan-in'];
const rows = stmt?.all(kind, limit);
const hotspots = rows.map((r) => ({
const stmt = HOTSPOT_QUERIES[metric] ?? HOTSPOT_QUERIES['fan-in'];
const rows = stmt.all(kind, limit) as {

Or, if strict null-checks are not flagging this already, at minimum add a guard:

const hotspots = (rows ?? []).map((r) => ({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — removed the superfluous \ on . Since \ is always concrete after the \ fallback to , the optional chaining was unnecessary and could widen the return type to include . Now uses direct \ call.

Comment on lines +66 to +78
for (let i = 0; i < exportKeys.length; i += EXPORT_CHUNK) {
const end = Math.min(i + EXPORT_CHUNK, exportKeys.length);
const chunkSize = end - i;
const conditions = Array.from(
{ length: chunkSize },
() => '(name = ? AND kind = ? AND file = ? AND line = ?)',
).join(' OR ');
const vals: unknown[] = [];
for (let j = i; j < end; j++) {
const k = exportKeys[j] as unknown[];
vals.push(k[0], k[1], k[2], k[3]);
}
db.prepare(`UPDATE nodes SET exported = 1 WHERE ${conditions}`).run(...vals);
Copy link
Contributor

Choose a reason for hiding this comment

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

P2 Export-marking UPDATE statement is re-prepared on every batch iteration

db.prepare(...) is called inside the loop, so for each chunk a new SQL string is parsed and a new prepared statement is compiled. This is inconsistent with the WeakMap-based caching introduced in helpers.ts (getNodeStmt / getEdgeStmt), which was added specifically to avoid per-chunk recompilation.

For a typical build the number of distinct chunk sizes is small (usually just 1 or 2), so the overhead is modest, but the inconsistency means this UPDATE still pays a preparation cost per batch iteration that the INSERT paths no longer do. Hoisting the statement cache outside the loop (or reusing the same pattern from helpers.ts) would close the gap:

// outside the loop:
const stmtCache = new Map<number, BetterSqlite3.Statement>();

// inside:
let updateStmt = stmtCache.get(chunkSize);
if (!updateStmt) {
  updateStmt = db.prepare(`UPDATE nodes SET exported = 1 WHERE ${conditions}`);
  stmtCache.set(chunkSize, updateStmt);
}
updateStmt.run(...vals);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed — added a Map<number, BetterSqlite3.Statement> cache (exportStmtCache) outside the loop body, keyed by chunk size. The prepared statement is now reused across iterations with the same chunk size, consistent with the WeakMap-based caching pattern in helpers.ts.

Impact: 21 functions changed, 39 affected
… stmt (#606)

Impact: 1 functions changed, 0 affected
@carlos-alm
Copy link
Contributor Author

@greptileai

…ity.ts (#606)

Impact: 3 functions changed, 2 affected
- Fix misleading "single nodeIdMap pass" comment in insertChildrenAndEdges
  (actually two passes: one before and one after batchInsertNodes)
- Cache role UPDATE prepared statements per chunk size in classifyNodeRoles,
  consistent with WeakMap-based caching pattern used in helpers.ts

Impact: 2 functions changed, 4 affected
@carlos-alm
Copy link
Contributor Author

Addressed the remaining items from the second Greptile review:

  1. Misleading comment in insertChildrenAndEdges — Fixed the function header comment from "single nodeIdMap pass" to "two nodeIdMap passes", and updated the inline comment at line 99 to accurately describe the first pass. The function does two bulkNodeIdsByFile calls per file (one before batchInsertNodes for children, one after to resolve child IDs for edges).

  2. Uncached role UPDATE prepared statements in classifyNodeRoles — Added a Map<number, SqliteStatement> cache (roleStmtCache) outside the inner loop, keyed by chunk size. The prepared statement is now reused across iterations with the same chunk size, consistent with the WeakMap-based caching pattern in helpers.ts.

  3. ?. in upsertPrecomputedComplexity — No change needed. The caller at line 543 guards with if (def.complexity), so def.complexity is non-nullable inside the function. The ?. on sub-properties like .halstead and .loc is defensive but harmless — these are always present when complexity exists. The cognitive and cyclomatic fields could theoretically pass undefined to SQL (becoming NULL), but this matches the column schema (nullable) and the native engine always provides them.

Commit: fix: correct misleading comments and cache role UPDATE stmts (#606)

@carlos-alm
Copy link
Contributor Author

@greptileai

Impact: 3 functions changed, 8 affected
@carlos-alm carlos-alm merged commit 0435c41 into main Mar 26, 2026
13 checks passed
@carlos-alm carlos-alm deleted the perf/native-bypass-and-batch-inserts branch March 26, 2026 03:04
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant