perf(incremental): batch WAL checkpoints and fix native CFG bulk insert#917
perf(incremental): batch WAL checkpoints and fix native CFG bulk insert#917carlos-alm merged 3 commits intomainfrom
Conversation
Replace per-feature wal_checkpoint(TRUNCATE) calls (~68ms each × 3-4 features) with a single upfront FULL checkpoint before analyses and one TRUNCATE after all analyses complete. suspendJsDb/resumeJsDb callbacks become no-ops, eliminating ~200-270ms of WAL checkpoint overhead on incremental rebuilds with the native engine. Also fix bulkInsertCfg silently failing due to napi-rs null→u32 conversion error: pass undefined (maps to Option::None) instead of null for optional CfgBlockRow fields (startLine, endLine, label).
|
Claude finished @carlos-alm's task —— View job 🔍 PR Review CompleteTodo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Performance bottleneck in incremental builds where WAL checkpoint operations were creating ~200-270ms overhead, and a silent failure in native CFG bulk insert due to null/undefined type mismatch. Root Cause Analysis:
Why This Approach: The solution addresses both root causes directly:
Risk Assessment: Low risk - this is a targeted performance optimization that maintains the same functional behavior while eliminating redundant WAL operations. Phase 2: Configuration Change Detection ✅✅ No configuration file changes detected - Only source code files modified:
✅ No linting, build, or test configuration modifications - Zero risk of silencing warnings or relaxing rules. Phase 3: Test Integrity Check ✅✅ Comprehensive test coverage verified:
✅ No red flags identified - Tests remain comprehensive and strict. Phase 4: Code Quality Review ✅✅ Implementation Analysis: WAL Checkpoint Optimization (
CFG Type Fix (
✅ Code style: Follows existing patterns, proper error handling, maintains existing logging format ✅ No security concerns: No injection vectors, proper resource cleanup Phase 5: Backlog Compliance Assessment ✅Zero-dep: ✅ - No new runtime dependencies added Foundation-aligned: ✅ - Fully compliant with all principles:
Problem-fit: 4/5 - Directly improves agent experience by reducing incremental build latency from 11.1ms to 5.7ms (-49%). Faster rebuilds mean fewer token-wasting delays when AI agents make changes. Breaking: No - Purely internal optimization, no user-facing API changes Tier: 1 (zero-dep ✅ + foundation-aligned ✅) Critical ConcernsNone identified. This is a well-executed performance optimization with proper testing. Minor Observations
Final Assessment |
Greptile SummaryTwo focused changes: WAL checkpoints are now batched (one Confidence Score: 5/5Safe to merge — both changes are correct and well-tested; only a one-line stale comment remains. The WAL batching logic is sound: JS writes are fully flushed before the Rust connection begins analysis, and the post-analyses TRUNCATE replaces the previous per-feature checkpoints symmetrically. The null→undefined fix correctly matches napi-rs Option semantics. All 12 incremental parity tests pass. The single remaining finding is a cosmetic comment mismatch (P2) that does not affect runtime behavior. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant JS as JS Pipeline (ctx.db)
participant Rust as Rust Engine (nativeDb)
participant WAL as SQLite WAL
Note over JS,WAL: buildEdges / buildStructure write JS rows into WAL
JS->>WAL: write edges, structure
JS->>Rust: reopenNativeDb()
JS->>WAL: wal_checkpoint(FULL) — flush JS WAL → DB file
Note over JS,Rust: suspendJsDb / resumeJsDb set to no-ops
loop runAnalyses (ast, complexity, CFG, dataflow)
Rust->>WAL: write analysis rows (bulkInsertCfg, etc.)
Note over JS,Rust: suspendJsDb() / resumeJsDb() are now no-ops
end
Rust->>WAL: wal_checkpoint(TRUNCATE) — flush Rust WAL → DB file
JS->>JS: refreshJsDb() — JS connection reads latest state
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/incr-checkp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Flush JS WAL pages once so Rust can see them, then no-op callbacks. | ||
| // Previously each feature called wal_checkpoint(TRUNCATE) individually | ||
| // (~68ms each × 3-4 features). One PASSIVE checkpoint suffices. | ||
| if (ctx.nativeDb && ctx.engineOpts) { | ||
| 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 (nativeDb may already be closed): ${toErrorMessage(e)}`, | ||
| ); | ||
| } | ||
| }; | ||
| ctx.db.pragma('wal_checkpoint(FULL)'); |
There was a problem hiding this comment.
FULL in code
The comment on line 496 says "One PASSIVE checkpoint suffices" but the pragma immediately below it uses wal_checkpoint(FULL). The same mismatch appears in runPipelineStages (~line 754–761): "do ONE upfront PASSIVE checkpoint" while the code calls wal_checkpoint(FULL). FULL is the correct choice here — unlike PASSIVE, it waits for existing readers to finish before checkpointing, giving a stronger visibility guarantee. The comments just need to be updated.
| // Flush JS WAL pages once so Rust can see them, then no-op callbacks. | |
| // Previously each feature called wal_checkpoint(TRUNCATE) individually | |
| // (~68ms each × 3-4 features). One PASSIVE checkpoint suffices. | |
| if (ctx.nativeDb && ctx.engineOpts) { | |
| 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 (nativeDb may already be closed): ${toErrorMessage(e)}`, | |
| ); | |
| } | |
| }; | |
| ctx.db.pragma('wal_checkpoint(FULL)'); | |
| // Flush JS WAL pages once so Rust can see them, then no-op callbacks. | |
| // Previously each feature called wal_checkpoint(TRUNCATE) individually | |
| // (~68ms each × 3-4 features). One FULL checkpoint suffices. | |
| if (ctx.nativeDb && ctx.engineOpts) { | |
| ctx.db.pragma('wal_checkpoint(FULL)'); |
There was a problem hiding this comment.
Fixed in c6f28a8 — updated both comments (line 496 and line 754) from "PASSIVE" to "FULL" to match the actual wal_checkpoint(FULL) pragma calls.
There was a problem hiding this comment.
Fixed in c6f28a8 — updated both comments (line 496 and line 754) from "PASSIVE" to "FULL" to match the actual wal_checkpoint(FULL) pragma calls.
Codegraph Impact Analysis3 functions changed → 9 callers affected across 7 files
|
1 participant
Summary
wal_checkpoint(TRUNCATE)calls (~68ms each x 3-4 features) with one upfrontFULLcheckpoint before analyses and oneTRUNCATEafter all complete.suspendJsDb/resumeJsDbcallbacks become no-ops, eliminating ~200-270ms of WAL overhead on incremental rebuilds with the native engine.bulkInsertCfgwas silently failing on every build — napi-rs can't convert JSnullto RustOption<u32>, onlyundefined. Changed?? nullto?? undefinedfor optionalCfgBlockRowfields (startLine,endLine,label). CFG data now persists via the native bulk path instead of falling back to WASM.Benchmark (53-file project, 1-file incremental, avg 3 runs)
Test plan