perf(native): fix full-build regression from NativeDbProxy overhead#906
perf(native): fix full-build regression from NativeDbProxy overhead#906carlos-alm wants to merge 2 commits intomainfrom
Conversation
…903) The NativeDbProxy introduced in #897 has per-statement napi serialization overhead (2 round-trips per .run()) that causes a full-build regression when multiplied across 667 files (9.4s vs 5.2s in 3.9.1). Three fixes: 1. Enable bulkInsertCfg native fast path in buildCFGData. The Rust method handles delete-before-insert atomically on a single rusqlite connection, eliminating the dual-connection WAL concern that originally blocked it. 2. After the Rust orchestrator completes, hand off to better-sqlite3 for JS post-processing (structure + analysis) instead of routing through the slow proxy. Wire suspendJsDb/resumeJsDb WAL callbacks and transfer the advisory lock to the new connection. 3. Optimize NativeDbProxy.run() to only query last_insert_rowid() for INSERT statements, halving per-statement overhead for DELETE/UPDATE. Closes #903
|
Claude finished @carlos-alm's task —— View job PR Review: Native Full-Build Performance FixReviewing this performance regression fix according to the 5-phase review process. Todo List
Root Cause AnalysisProblem Being Solved: Native engine full-build regression from 5.2s (v3.9.1) to 9.4s (v3.9.2), an 81% slowdown caused by NativeDbProxy per-statement napi serialization overhead. Why This Approach: The regression was introduced by #897's NativeDbProxy which adds 2 round-trips per SQL
Risk Assessment: LOW - These are surgical optimizations that don't change core logic, only reduce unnecessary overhead. Code Quality Analysisnative-db-proxy.ts:30-48 ✅ Well-designed optimizationconst isInsert = sql.trimStart().substring(0, 6).toUpperCase() === 'INSERT';
// ... later in run()
if (isInsert) {
const row = ndb.queryGet('SELECT last_insert_rowid() AS rid', []) as {
rid: number;
} | null;
return { changes: 0, lastInsertRowid: row?.rid ?? 0 };
}
return { changes: 0, lastInsertRowid: 0 };
pipeline.ts:652-668 ✅ Proper handoff logic// Always hand off to better-sqlite3 for JS post-processing.
// The NativeDbProxy has per-statement napi serialization overhead that
// makes structure/analysis phases significantly slower than direct better-sqlite3.
const lockPath = ctx.nativeFirstProxy
? (ctx.db as unknown as { __lockPath?: string }).__lockPath
: undefined;
if (ctx.nativeFirstProxy) ctx.nativeFirstProxy = false;
if (!handoffWalAfterNativeBuild(ctx)) {
return formatNativeTimingResult(p, 0, analysisTiming);
}
// Transfer advisory lock ownership from proxy to new better-sqlite3 connection
if (lockPath) (ctx.db as unknown as { __lockPath?: string }).__lockPath = lockPath;
cfg.ts:386-430 ✅ Native bulk-insert enablementif (allNative && nativeDb?.bulkInsertCfg) {
// ... build entries array
let inserted: number;
try {
engineOpts?.suspendJsDb?.();
inserted = nativeDb.bulkInsertCfg(entries);
} finally {
engineOpts?.resumeJsDb?.();
}
info(`CFG (native bulk): ${inserted} functions analyzed`);
}
Backlog Compliance
Test Plan AssessmentThe PR shows partial test completion:
Final Recommendation
Conditions for merge:
|
Codegraph Impact Analysis6 functions changed → 11 callers affected across 7 files
|
Greptile SummaryFixes the 3.9.2 full-build regression (~81% slowdown) caused by per-statement napi serialization overhead in Confidence Score: 5/5Safe to merge — the WAL handoff and bulk-insert fast path are correctly implemented; remaining findings are P2 cleanup items. All findings are P2 (stale comment, theoretical INSERT-detection gap). The core WAL suspend/resume lifecycle, the proxy-to-better-sqlite3 handoff, and the bulkInsertCfg gating are logically sound. No data-integrity or correctness issues found. pipeline.ts — stale comment on runPostNativeAnalysis (line 523) worth cleaning up before this pattern spreads.
|
| Filename | Overview |
|---|---|
| src/domain/graph/builder/native-db-proxy.ts | Optimizes .run() to skip last_insert_rowid query for non-INSERT statements; minor edge-case gap for CTE-prefixed inserts and REPLACE INTO. |
| src/domain/graph/builder/pipeline.ts | Always hands off from NativeDbProxy to better-sqlite3 after the orchestrator; correctly wires suspendJsDb/resumeJsDb; stale comment and dead !ctx.nativeFirstProxy guard remain in runPostNativeAnalysis. |
| src/features/cfg.ts | Enables the native bulkInsertCfg fast path (previously blocked), correctly scoped behind allNative && nativeDb?.bulkInsertCfg with WAL suspend/resume guards. |
Sequence Diagram
sequenceDiagram
participant O as tryNativeOrchestrator
participant R as Rust buildGraph
participant H as handoffWalAfterNativeBuild
participant JS as better-sqlite3 (ctx.db)
participant NA as NativeDb (analysis)
participant CFG as buildCFGData
O->>R: buildGraph() via nativeDb
R-->>O: result (nodes/edges)
O->>H: always hand off (nativeFirstProxy=false)
H->>H: closeNativeDb (WAL checkpoint + close)
H->>JS: openDb() — fresh better-sqlite3
O->>NA: runPostNativeAnalysis()
NA->>NA: openReadWrite (fresh nativeDb for analysis)
NA->>NA: wire suspendJsDb / resumeJsDb
NA->>CFG: buildCFGData(db, fileSymbols, engineOpts)
CFG->>JS: getFunctionNodeId() reads
CFG->>JS: suspendJsDb() — WAL checkpoint
CFG->>NA: bulkInsertCfg(entries) — atomic delete+insert
CFG->>NA: resumeJsDb() — WAL checkpoint
NA->>NA: close nativeDb after analyses
O->>O: closeDbPair(ctx.db, ctx.nativeDb)
Reviews (2): Last reviewed commit: "fix: address review feedback — remove re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/domain/graph/builder/pipeline.ts
Outdated
| const lockPath = ctx.nativeFirstProxy | ||
| ? (ctx.db as unknown as { __lockPath?: string }).__lockPath | ||
| : undefined; | ||
| if (ctx.nativeFirstProxy) ctx.nativeFirstProxy = false; | ||
| if (!handoffWalAfterNativeBuild(ctx)) { | ||
| // DB reopen failed — return partial result | ||
| return formatNativeTimingResult(p, 0, analysisTiming); | ||
| } | ||
| // Transfer advisory lock ownership from the proxy to the new better-sqlite3 | ||
| // connection so closeDbPair releases it at the end. | ||
| if (lockPath) (ctx.db as unknown as { __lockPath?: string }).__lockPath = lockPath; |
There was a problem hiding this comment.
openDb() always calls acquireAdvisoryLock(dbPath) and then immediately sets db.__lockPath = \${dbPath}.lock`on the returned connection (seeconnection.ts:165–170). The transfer at line 668 re-assigns the same string that was already written by openDb, so it is a no-op. The lockPath` capture and conditional assign can be removed to reduce confusion about ownership semantics.
| const lockPath = ctx.nativeFirstProxy | |
| ? (ctx.db as unknown as { __lockPath?: string }).__lockPath | |
| : undefined; | |
| if (ctx.nativeFirstProxy) ctx.nativeFirstProxy = false; | |
| if (!handoffWalAfterNativeBuild(ctx)) { | |
| // DB reopen failed — return partial result | |
| return formatNativeTimingResult(p, 0, analysisTiming); | |
| } | |
| // Transfer advisory lock ownership from the proxy to the new better-sqlite3 | |
| // connection so closeDbPair releases it at the end. | |
| if (lockPath) (ctx.db as unknown as { __lockPath?: string }).__lockPath = lockPath; | |
| if (ctx.nativeFirstProxy) ctx.nativeFirstProxy = false; | |
| if (!handoffWalAfterNativeBuild(ctx)) { | |
| // DB reopen failed — return partial result | |
| return formatNativeTimingResult(p, 0, analysisTiming); | |
| } | |
There was a problem hiding this comment.
Fixed — removed the redundant lockPath capture and conditional transfer. As noted, openDb() already calls acquireAdvisoryLock(dbPath) and sets __lockPath on the new connection (connection.ts:165-170), so the transfer was a no-op.
| if (entries.length > 0) { | ||
| let inserted: number; | ||
| try { | ||
| engineOpts?.suspendJsDb?.(); | ||
| inserted = nativeDb.bulkInsertCfg(entries); | ||
| } finally { | ||
| engineOpts?.resumeJsDb?.(); | ||
| } | ||
| info(`CFG (native bulk): ${inserted} functions analyzed`); | ||
| } |
There was a problem hiding this comment.
inserted is uninitialized if suspendJsDb throws before bulkInsertCfg runs
TypeScript accepts this because a throw inside the try block means info() is unreachable, so the compiler knows inserted is assigned when info() is actually executed. Runtime behaviour is correct. However, initialising inserted = 0 makes the intent explicit and avoids potential confusion if a catch block is ever added later.
| if (entries.length > 0) { | |
| let inserted: number; | |
| try { | |
| engineOpts?.suspendJsDb?.(); | |
| inserted = nativeDb.bulkInsertCfg(entries); | |
| } finally { | |
| engineOpts?.resumeJsDb?.(); | |
| } | |
| info(`CFG (native bulk): ${inserted} functions analyzed`); | |
| } | |
| if (entries.length > 0) { | |
| let inserted = 0; | |
| try { | |
| engineOpts?.suspendJsDb?.(); | |
| inserted = nativeDb.bulkInsertCfg(entries); | |
| } finally { | |
| engineOpts?.resumeJsDb?.(); | |
| } | |
| info(`CFG (native bulk): ${inserted} functions analyzed`); | |
| } |
There was a problem hiding this comment.
Fixed — initialized inserted = 0 for explicit intent, matching the suggestion exactly.
…d dead branch (#906) - Remove redundant lockPath capture and transfer in tryNativeOrchestrator (openDb already sets __lockPath on the new connection) - Remove dead else-if branch in runPostNativeAnalysis (ctx.nativeFirstProxy is always false after the unconditional reset in tryNativeOrchestrator) - Initialize inserted = 0 in buildCFGData for explicit intent
|
Addressed Greptile's outside-diff comment (P2: dead branch at pipeline.ts:504-506): removed the |
NativeDbProxy overhead causes native full build to regress +81% (5206ms -> 9403ms). Fix tracked in PR #906. Add to KNOWN_REGRESSIONS to unblock this benchmark data PR.
1 participant
Summary
Fixes #903 — native full-build regression in 3.9.2 (9.4s vs 5.2s in 3.9.1, +81%).
The
NativeDbProxyintroduced in #897 has per-statement napi serialization overhead (2 round-trips per.run()) that dominates when multiplied across 667 files × thousands of SQL operations in structure/analysis phases.Three fixes:
bulkInsertCfgnative fast path (cfg.ts): The Rust method handles delete-before-insert atomically on a single rusqlite connection, eliminating the dual-connection WAL concern that originally blocked it. Replaces hundreds of individual proxy SQL calls with a single napi bulk call.pipeline.ts): After the Rust orchestrator completes, switch from proxy to direct better-sqlite3 for JS post-processing (structure + analysis). Properly wiressuspendJsDb/resumeJsDbWAL callbacks and transfers the advisory lock to the new connection.NativeDbProxy.run()(native-db-proxy.ts): Only querylast_insert_rowid()for INSERT statements — DELETE/UPDATE skip the extra napi call, halving per-statement overhead.Expected impact
Test plan
tsc --noEmit)