-
Notifications
You must be signed in to change notification settings - Fork 11
fix(native): js-side fast-skip for incremental no-op rebuilds (#1054) #1064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
e5510e9
8699471
540a420
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -512,6 +512,63 @@ function handleIncrementalBuild(ctx: PipelineContext): void { | |
| purgeAndAddReverseDeps(ctx, changePaths, reverseDeps); | ||
| } | ||
|
|
||
| /** | ||
| * Read-only pre-flight check for the native orchestrator. | ||
| * | ||
| * Returns true iff every collected source file has matching mtime+size in | ||
| * `file_hashes` and no DB-tracked file has been removed. When true, the | ||
| * caller can short-circuit before invoking the native orchestrator — | ||
| * matching WASM's ~20 ms early-exit path and avoiding the ~2s flat | ||
| * per-call native rebuild overhead seen in CI (#1054). | ||
| * | ||
| * Intentionally Tier-0/Tier-1 only (journal + mtime/size). Tier-2 content | ||
| * hashing is left to the native side: when this returns false the caller | ||
| * falls through to the orchestrator, which performs its own complete | ||
| * detection and is the source of truth. | ||
| * | ||
| * Pure read of `db` and the filesystem — never mutates either. | ||
| */ | ||
| export function detectNoChanges( | ||
| db: BetterSqlite3Database, | ||
| allFiles: string[], | ||
| rootDir: string, | ||
| ): boolean { | ||
| let hasTable = false; | ||
| try { | ||
| db.prepare('SELECT 1 FROM file_hashes LIMIT 1').get(); | ||
| hasTable = true; | ||
| } catch { | ||
| /* table missing — first build */ | ||
| } | ||
| if (!hasTable) return false; | ||
|
|
||
| const rows = db.prepare('SELECT file, hash, mtime, size FROM file_hashes').all() as FileHashRow[]; | ||
| if (rows.length === 0) return false; | ||
| const existing = new Map<string, FileHashRow>(rows.map((r) => [r.file, r])); | ||
|
|
||
| const currentFiles = new Set<string>(); | ||
| for (const file of allFiles) { | ||
| currentFiles.add(normalizePath(path.relative(rootDir, file))); | ||
| } | ||
| for (const existingFile of existing.keys()) { | ||
| if (!currentFiles.has(existingFile)) return false; | ||
| } | ||
|
|
||
| for (const file of allFiles) { | ||
| const relPath = normalizePath(path.relative(rootDir, file)); | ||
| const record = existing.get(relPath); | ||
| if (!record) return false; | ||
| const stat = fileStat(file) as FileStat | undefined; | ||
| if (!stat) return false; | ||
| const storedMtime = record.mtime || 0; | ||
| const storedSize = record.size || 0; | ||
| if (storedSize <= 0) return false; | ||
| if (Math.floor(stat.mtimeMs) !== storedMtime || stat.size !== storedSize) return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
Comment on lines
512
to
+586
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The existing JS path guards this explicitly: it calls
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 8699471.
This mirrors the same empty-table check that The pipeline.ts call site now passes |
||
|
|
||
| export async function detectChanges(ctx: PipelineContext): Promise<void> { | ||
| const start = performance.now(); | ||
| try { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectFilescall on fallthrough torunPipelineStagesWhen
detectNoChangesreturnsfalse(changes detected), control falls through totryNativeOrchestrator. If the native orchestrator itself fails andrunPipelineStagesis reached,collectFilesis called again at line 901 — doubling the collection work on this path.ctx.allFilesis overwritten with identical data, so there is no functional bug, but the extra traversal adds latency that counteracts the PR's goal of eliminating unnecessary overhead on the non-skip path. Consider guarding the secondcollectFilescall inrunPipelineStageswith a check likeif (!ctx.allFiles?.length), or resettingctx.allFilesin the catch block above so the second call is always the canonical one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 540a420. Guarded the
collectFilesstage so it returns early whenctx.allFilesandctx.discoveredDirsare already populated (andopts.scopeis unset). On the fallthrough path (pre-flight ran but changes were detected → tryNativeOrchestrator → JS pipeline fallback),runPipelineStagesnow reuses the already-collected file list instead of re-walking the filesystem.To keep the guard correct on the failure path, the buildGraph catch block now resets
ctx.allFilesandctx.discoveredDirsto undefined when the pre-flight throws — sorunPipelineStagesre-collects under its own engine state if we got partially populated before the error.