perf(builder): defer NativeDatabase init to after no-op early exit#884
perf(builder): defer NativeDatabase init to after no-op early exit#884carlos-alm merged 2 commits intomainfrom
Conversation
On no-op rebuilds, the native engine opened NativeDatabase, ran initSchema, checkpointed WAL, then immediately closed it — all before detectChanges even ran. This added ~5ms overhead on every no-op rebuild where the connection was never used. Move NativeDatabase.openReadWrite() from setupPipeline to on-demand: - tryNativeOrchestrator opens it when the Rust fast path will run - runPipelineStages defers it until after the early-exit check - setupPipeline now only probes loadNative() for availability - checkEngineSchemaMismatch uses better-sqlite3 (reads are WAL-safe) - Remove dead batched native path from getChangedFiles (nativeDb was already undefined during detectChanges due to suspendNativeDb)
|
Claude finished @carlos-alm's task —— View job PR Review: perf(builder): defer NativeDatabase init to after no-op early exit ⭐⭐⭐⭐☆Root Cause AnalysisProblem Being Solved: No-op rebuilds (incremental builds with no file changes) were unnecessarily spending ~5ms to open, initialize schema, and close the native database connection, even when the build would exit early without using it. This created a performance disparity between WASM and native engine paths on no-op rebuilds. Why This Approach: The solution defers
Risk Assessment: Low risk. The changes maintain the same functional behavior while optimizing the common no-op path. The deferral is well-architected with proper error handling and fallback mechanisms. Backlog Compliance
Code Quality AssessmentPositive aspects:
Technical correctness:
Performance impact:
Test CoverageThe PR author reports comprehensive test coverage:
The existing Minor Concerns
Critical ConcernsNone. The implementation is solid and well-tested. Final Recommendation
The 40-line net reduction while improving performance demonstrates good engineering practices. The deferral strategy is appropriate and maintains the existing architectural patterns. |
Greptile SummaryThis PR defers One regression: Confidence Score: 4/5Not safe to merge without addressing the WAL guard regression in the native orchestrator post-analysis path. The prior WAL callback regression is correctly fixed for the JS pipeline path. However,
Important Files Changed
Sequence DiagramsequenceDiagram
participant BG as buildGraph()
participant SP as setupPipeline
participant TNO as tryNativeOrchestrator
participant RPNA as runPostNativeAnalysis
participant RPS as runPipelineStages
participant RA as runAnalyses
BG->>SP: setup (nativeAvailable=true, nativeDb=undefined)
BG->>TNO: try native orchestrator
TNO->>TNO: open NativeDatabase on demand
TNO->>TNO: buildGraph (Rust)
alt earlyExit (no-op)
TNO-->>BG: 'early-exit' (NativeDB never opened ✓)
else native success
TNO->>RPNA: runPostNativeAnalysis
RPNA->>RPNA: reopen nativeDb
RPNA->>RPNA: set engineOpts.nativeDb only
Note over RPNA: ⚠️ suspendJsDb/resumeJsDb remain undefined
RPNA->>RA: runAnalyses (WAL guard missing)
TNO-->>BG: BuildResult
else native fail / FORCE_JS_PIPELINE
TNO-->>BG: undefined
BG->>RPS: runPipelineStages
RPS->>RPS: collectFiles + detectChanges (nativeDb=undefined)
alt earlyExit (no-op)
RPS-->>BG: return (NativeDB never opened ✓)
else files changed
RPS->>RPS: reopenNativeDb('analyses')
RPS->>RPS: set nativeDb + suspendJsDb + resumeJsDb ✓
RPS->>RA: runAnalyses (WAL guard active)
end
end
Reviews (2): Last reviewed commit: "fix(builder): restore WAL checkpoint cal..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // nativeDb and WAL callbacks are set later when NativeDatabase is opened | ||
| // (deferred to skip overhead on no-op rebuilds). | ||
| nativeDb: undefined, | ||
| suspendJsDb: undefined, | ||
| resumeJsDb: undefined, |
There was a problem hiding this comment.
These fields are set to undefined here with a comment promising they'll be populated "later when NativeDatabase is opened", but the only later assignment — the if (ctx.nativeAvailable) block in runPipelineStages (~line 660) — only sets ctx.engineOpts.nativeDb. The WAL callbacks are never assigned. When the JS pipeline runs with native engine active (e.g., forceFullRebuild triggered by schema/engine/version change, or CODEGRAPH_FORCE_JS_PIPELINE=1), ast.ts, complexity.ts, cfg.ts, and dataflow.ts all invoke nativeDb.bulkInsert* wrapped in suspendJsDb?.() / resumeJsDb?.() — both silently no-op, removing the WAL checkpoint guard introduced in #696/#715 that prevents dual-connection corruption when rusqlite and better-sqlite3 are open to the same WAL file simultaneously.
The fix is to assign both callbacks in the if (ctx.nativeDb && ctx.engineOpts) branch after reopenNativeDb(ctx, 'analyses'):
if (ctx.nativeDb && ctx.engineOpts) {
ctx.engineOpts.nativeDb = ctx.nativeDb;
ctx.engineOpts.suspendJsDb = () => {
ctx.db.pragma('wal_checkpoint(TRUNCATE)');
};
ctx.engineOpts.resumeJsDb = () => {
try {
ctx.nativeDb?.exec('PRAGMA wal_checkpoint(TRUNCATE)');
} catch (e) {
debug(`resumeJsDb: WAL checkpoint failed: ${toErrorMessage(e)}`);
}
};
}There was a problem hiding this comment.
Fixed in 1ef71db. Both suspendJsDb and resumeJsDb are now assigned in the reopenNativeDb('analyses') block alongside engineOpts.nativeDb. Also explicitly resets both to undefined in the fallback branch when nativeDb is unavailable. Good catch — this was a real regression of the WAL corruption guard from #696/#715.
Codegraph Impact Analysis9 functions changed → 10 callers affected across 6 files
|
…884) suspendJsDb and resumeJsDb were initialized to undefined in initializeEngine and never reassigned when nativeDb was reopened for the analysis phase. This silently removed the dual-connection WAL corruption guard introduced in #696/#715. Assign both callbacks alongside engineOpts.nativeDb in the reopenNativeDb('analyses') block.
1 participant
Summary
NativeDatabase.openReadWrite()+initSchema()fromsetupPipelineto on-demand: the native orchestrator opens it when the Rust fast path runs, and the JS pipeline defers until after the early-exit checkgetChangedFiles(nativeDbwas alreadyundefinedduringdetectChangesdue tosuspendNativeDb)Test plan
tests/builder/pipeline.test.ts— 4/4 pass (including no-op rebuild test)tests/builder/detect-changes.test.ts— 4/4 passtests/integration/build.test.ts— 17/17 pass (2 skipped, 1 pre-existing worktree WASM grammar issue)tests/integration/incremental-parity.test.ts— 12/12 passtests/integration/watcher-rebuild.test.ts— passtests/builder/context.test.ts— pass