From 5811b15fb9112ac24b5de43a819d6f0cccf8b50d Mon Sep 17 00:00:00 2001 From: carlos-alm <127798846+carlos-alm@users.noreply.github.com> Date: Tue, 21 Apr 2026 00:16:11 -0600 Subject: [PATCH 1/3] feat(build): report collectMs/detectMs as separate phases (#977) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split collect and detect-change work out of setupMs in BuildResult.phases. These two phases have very different cost profiles (file-walk + ignore matching vs hash/mtime checks), and collapsing them hid where time was actually being spent during incremental-build perf investigations. - types.ts: add collectMs/detectMs to BuildResult.phases - context.ts: add collectMs/detectMs to ctx.timing - collect-files.ts, detect-changes.ts: wrap exported entries with performance.now() (try/finally so early returns still record timing) - pipeline.ts: formatTimingResult and formatNativeTimingResult surface the two fields separately instead of folding native-side values back into setupMs Rust PipelineTiming already tracks collect_ms and detect_ms, so no new instrumentation on the native side. docs check acknowledged — observability-only change, no README/CLAUDE/ ROADMAP impact. Impact: 6 functions changed, 7 affected --- src/domain/graph/builder/context.ts | 2 + src/domain/graph/builder/pipeline.ts | 6 +- .../graph/builder/stages/collect-files.ts | 76 +++++++------ .../graph/builder/stages/detect-changes.ts | 104 +++++++++--------- src/types.ts | 2 + tests/builder/pipeline.test.ts | 2 + 6 files changed, 107 insertions(+), 85 deletions(-) diff --git a/src/domain/graph/builder/context.ts b/src/domain/graph/builder/context.ts index 70752698..330aa0e3 100644 --- a/src/domain/graph/builder/context.ts +++ b/src/domain/graph/builder/context.ts @@ -87,6 +87,8 @@ export class PipelineContext { // ── Phase timing ─────────────────────────────────────────────────── timing: { setupMs?: number; + collectMs?: number; + detectMs?: number; parseMs?: number; insertMs?: number; resolveMs?: number; diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index b9c20524..2a395f60 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -184,6 +184,8 @@ function formatTimingResult(ctx: PipelineContext): BuildResult { return { phases: { setupMs: +(t.setupMs ?? 0).toFixed(1), + collectMs: +(t.collectMs ?? 0).toFixed(1), + detectMs: +(t.detectMs ?? 0).toFixed(1), parseMs: +(t.parseMs ?? 0).toFixed(1), insertMs: +(t.insertMs ?? 0).toFixed(1), resolveMs: +(t.resolveMs ?? 0).toFixed(1), @@ -558,7 +560,9 @@ function formatNativeTimingResult( ): BuildResult { return { phases: { - setupMs: +((p.setupMs ?? 0) + (p.collectMs ?? 0) + (p.detectMs ?? 0)).toFixed(1), + setupMs: +(p.setupMs ?? 0).toFixed(1), + collectMs: +(p.collectMs ?? 0).toFixed(1), + detectMs: +(p.detectMs ?? 0).toFixed(1), parseMs: +(p.parseMs ?? 0).toFixed(1), insertMs: +(p.insertMs ?? 0).toFixed(1), resolveMs: +(p.resolveMs ?? 0).toFixed(1), diff --git a/src/domain/graph/builder/stages/collect-files.ts b/src/domain/graph/builder/stages/collect-files.ts index 73c19441..700e5d3a 100644 --- a/src/domain/graph/builder/stages/collect-files.ts +++ b/src/domain/graph/builder/stages/collect-files.ts @@ -7,6 +7,7 @@ */ import fs from 'node:fs'; import path from 'node:path'; +import { performance } from 'node:perf_hooks'; import { debug, info } from '../../../../infrastructure/logger.js'; import { normalizePath } from '../../../../shared/constants.js'; import { readJournal } from '../../journal.js'; @@ -86,45 +87,50 @@ function tryFastCollect( } export async function collectFiles(ctx: PipelineContext): Promise { - const { rootDir, config, opts } = ctx; + const start = performance.now(); + try { + const { rootDir, config, opts } = ctx; - if (opts.scope) { - // Scoped rebuild: rebuild only specified files - const scopedFiles = opts.scope.map((f: string) => normalizePath(f)); - const existing: Array<{ file: string; relPath: string }> = []; - const missing: string[] = []; - for (const rel of scopedFiles) { - const abs = path.join(rootDir, rel); - if (fs.existsSync(abs)) { - existing.push({ file: abs, relPath: rel }); - } else { - missing.push(rel); + if (opts.scope) { + // Scoped rebuild: rebuild only specified files + const scopedFiles = opts.scope.map((f: string) => normalizePath(f)); + const existing: Array<{ file: string; relPath: string }> = []; + const missing: string[] = []; + for (const rel of scopedFiles) { + const abs = path.join(rootDir, rel); + if (fs.existsSync(abs)) { + existing.push({ file: abs, relPath: rel }); + } else { + missing.push(rel); + } } + ctx.allFiles = existing.map((e) => e.file); + ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file))); + ctx.parseChanges = existing; + ctx.metadataUpdates = []; + ctx.removed = missing; + ctx.isFullBuild = false; + info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`); + return; } - ctx.allFiles = existing.map((e) => e.file); - ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file))); - ctx.parseChanges = existing; - ctx.metadataUpdates = []; - ctx.removed = missing; - ctx.isFullBuild = false; - info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`); - return; - } - // Incremental fast path: reconstruct file list from DB + journal deltas - // instead of full recursive filesystem scan (~8ms savings on 473 files). - if (ctx.incremental && !ctx.forceFullRebuild) { - const fast = tryFastCollect(ctx); - if (fast) { - ctx.allFiles = fast.files; - ctx.discoveredDirs = fast.directories; - info(`Found ${ctx.allFiles.length} files (cached)`); - return; + // Incremental fast path: reconstruct file list from DB + journal deltas + // instead of full recursive filesystem scan (~8ms savings on 473 files). + if (ctx.incremental && !ctx.forceFullRebuild) { + const fast = tryFastCollect(ctx); + if (fast) { + ctx.allFiles = fast.files; + ctx.discoveredDirs = fast.directories; + info(`Found ${ctx.allFiles.length} files (cached)`); + return; + } } - } - const collected = collectFilesUtil(rootDir, [], config, new Set()); - ctx.allFiles = collected.files; - ctx.discoveredDirs = collected.directories; - info(`Found ${ctx.allFiles.length} files to parse`); + const collected = collectFilesUtil(rootDir, [], config, new Set()); + ctx.allFiles = collected.files; + ctx.discoveredDirs = collected.directories; + info(`Found ${ctx.allFiles.length} files to parse`); + } finally { + ctx.timing.collectMs = performance.now() - start; + } } diff --git a/src/domain/graph/builder/stages/detect-changes.ts b/src/domain/graph/builder/stages/detect-changes.ts index 1b5d612e..dfd4ad5b 100644 --- a/src/domain/graph/builder/stages/detect-changes.ts +++ b/src/domain/graph/builder/stages/detect-changes.ts @@ -7,6 +7,7 @@ */ import fs from 'node:fs'; import path from 'node:path'; +import { performance } from 'node:perf_hooks'; import { closeDb } from '../../../../db/index.js'; import { debug, info } from '../../../../infrastructure/logger.js'; import { normalizePath } from '../../../../shared/constants.js'; @@ -512,59 +513,64 @@ function handleIncrementalBuild(ctx: PipelineContext): void { } export async function detectChanges(ctx: PipelineContext): Promise { - const { db, allFiles, rootDir, incremental, forceFullRebuild, opts } = ctx; - if ((opts as Record).scope) { - handleScopedBuild(ctx); - return; - } - const increResult = - incremental && !forceFullRebuild - ? getChangedFiles(db, allFiles, rootDir) - : { - changed: allFiles.map((f): ChangedFile => ({ file: f })), - removed: [] as string[], - isFullBuild: true, - }; - ctx.removed = increResult.removed; - ctx.isFullBuild = increResult.isFullBuild; - ctx.parseChanges = increResult.changed - .filter((c) => !c.metadataOnly) - .map((c) => ({ - file: c.file, - relPath: c.relPath, - content: c.content, - hash: c.hash, - stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined, - _reverseDepOnly: c._reverseDepOnly, - })); - ctx.metadataUpdates = increResult.changed - .filter( - (c): c is ChangedFile & { relPath: string; hash: string; stat: FileStat } => - !!c.metadataOnly && !!c.relPath && !!c.hash && !!c.stat, - ) - .map((c) => ({ - relPath: c.relPath, - hash: c.hash, - stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size }, - })); - if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) { - const ranAnalysis = await runPendingAnalysis(ctx); - if (ranAnalysis) { + const start = performance.now(); + try { + const { db, allFiles, rootDir, incremental, forceFullRebuild, opts } = ctx; + if ((opts as Record).scope) { + handleScopedBuild(ctx); + return; + } + const increResult = + incremental && !forceFullRebuild + ? getChangedFiles(db, allFiles, rootDir) + : { + changed: allFiles.map((f): ChangedFile => ({ file: f })), + removed: [] as string[], + isFullBuild: true, + }; + ctx.removed = increResult.removed; + ctx.isFullBuild = increResult.isFullBuild; + ctx.parseChanges = increResult.changed + .filter((c) => !c.metadataOnly) + .map((c) => ({ + file: c.file, + relPath: c.relPath, + content: c.content, + hash: c.hash, + stat: c.stat ? { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size } : undefined, + _reverseDepOnly: c._reverseDepOnly, + })); + ctx.metadataUpdates = increResult.changed + .filter( + (c): c is ChangedFile & { relPath: string; hash: string; stat: FileStat } => + !!c.metadataOnly && !!c.relPath && !!c.hash && !!c.stat, + ) + .map((c) => ({ + relPath: c.relPath, + hash: c.hash, + stat: { mtime: Math.floor(c.stat.mtimeMs), size: c.stat.size }, + })); + if (!ctx.isFullBuild && ctx.parseChanges.length === 0 && ctx.removed.length === 0) { + const ranAnalysis = await runPendingAnalysis(ctx); + if (ranAnalysis) { + closeDb(db); + writeJournalHeader(rootDir, Date.now()); + ctx.earlyExit = true; + return; + } + healMetadata(ctx); + info('No changes detected. Graph is up to date.'); closeDb(db); writeJournalHeader(rootDir, Date.now()); ctx.earlyExit = true; return; } - healMetadata(ctx); - info('No changes detected. Graph is up to date.'); - closeDb(db); - writeJournalHeader(rootDir, Date.now()); - ctx.earlyExit = true; - return; - } - if (ctx.isFullBuild) { - handleFullBuild(ctx); - } else { - handleIncrementalBuild(ctx); + if (ctx.isFullBuild) { + handleFullBuild(ctx); + } else { + handleIncrementalBuild(ctx); + } + } finally { + ctx.timing.detectMs = performance.now() - start; } } diff --git a/src/types.ts b/src/types.ts index 09159205..d35b6382 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1053,6 +1053,8 @@ export interface BuildGraphOpts { export interface BuildResult { phases: { setupMs: number; + collectMs: number; + detectMs: number; parseMs: number; insertMs: number; resolveMs: number; diff --git a/tests/builder/pipeline.test.ts b/tests/builder/pipeline.test.ts index 350808fb..a3aee31a 100644 --- a/tests/builder/pipeline.test.ts +++ b/tests/builder/pipeline.test.ts @@ -31,6 +31,8 @@ describe('buildGraph pipeline', () => { expect(result).toBeDefined(); expect(result.phases).toBeDefined(); expect(typeof result.phases.setupMs).toBe('number'); + expect(typeof result.phases.collectMs).toBe('number'); + expect(typeof result.phases.detectMs).toBe('number'); expect(typeof result.phases.parseMs).toBe('number'); expect(typeof result.phases.insertMs).toBe('number'); expect(typeof result.phases.resolveMs).toBe('number'); From bf3c3d3a8e85377a8f4609884663d48be4985a97 Mon Sep 17 00:00:00 2001 From: carlos-alm <127798846+carlos-alm@users.noreply.github.com> Date: Tue, 21 Apr 2026 01:12:50 -0600 Subject: [PATCH 2/3] fix(types): align PipelineContext.timing interface with class definition (#993) Impact: 1 functions changed, 0 affected --- src/types.ts | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/types.ts b/src/types.ts index d35b6382..b5614c73 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1033,7 +1033,23 @@ export interface PipelineContext { lineCountMap: Map; // Phase timing - timing: Record; + timing: { + setupMs?: number; + collectMs?: number; + detectMs?: number; + parseMs?: number; + insertMs?: number; + resolveMs?: number; + edgesMs?: number; + structureMs?: number; + rolesMs?: number; + astMs?: number; + complexityMs?: number; + cfgMs?: number; + dataflowMs?: number; + finalizeMs?: number; + [key: string]: number | undefined; + }; buildStart: number; } From 87ce95dadf66bbdcf8a06c165226b45df5b0a3e9 Mon Sep 17 00:00:00 2001 From: carlos-alm <127798846+carlos-alm@users.noreply.github.com> Date: Wed, 22 Apr 2026 00:29:19 -0600 Subject: [PATCH 3/3] refactor(build): separate collectMs from detectMs on scoped path (#993) Addresses Greptile P2 feedback on #993: - collectMs no longer includes change-detection work on the scoped-rebuild path. Only filesystem-walk work (existence checks + file list) is timed under collectMs; parseChanges/removed/isFullBuild assignments are timed under detectMs for semantic consistency with the non-scoped path. - detectChanges now accumulates detectMs additively to respect the partial contribution from collectFiles on the scoped path. - Pipeline test asserts collectMs and detectMs are >= 0, not just 'number', so a silent regression where timing is never recorded would be caught. Impact: 2 functions changed, 5 affected --- .../graph/builder/stages/collect-files.ts | 41 ++++++++++++------- .../graph/builder/stages/detect-changes.ts | 4 +- tests/builder/pipeline.test.ts | 2 + 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/domain/graph/builder/stages/collect-files.ts b/src/domain/graph/builder/stages/collect-files.ts index 700e5d3a..92e55f32 100644 --- a/src/domain/graph/builder/stages/collect-files.ts +++ b/src/domain/graph/builder/stages/collect-files.ts @@ -87,15 +87,20 @@ function tryFastCollect( } export async function collectFiles(ctx: PipelineContext): Promise { - const start = performance.now(); - try { - const { rootDir, config, opts } = ctx; + const { rootDir, config, opts } = ctx; - if (opts.scope) { - // Scoped rebuild: rebuild only specified files - const scopedFiles = opts.scope.map((f: string) => normalizePath(f)); - const existing: Array<{ file: string; relPath: string }> = []; - const missing: string[] = []; + if (opts.scope) { + // Scoped rebuild: rebuild only specified files. + // + // Timer only wraps the filesystem-walk portion (existence checks + file + // list construction). Change-detection outputs (parseChanges, removed, + // isFullBuild) are attributed to detectMs for semantic consistency with + // the non-scoped path, even though this stage computes them. + const start = performance.now(); + const scopedFiles = opts.scope.map((f: string) => normalizePath(f)); + const existing: Array<{ file: string; relPath: string }> = []; + const missing: string[] = []; + try { for (const rel of scopedFiles) { const abs = path.join(rootDir, rel); if (fs.existsSync(abs)) { @@ -106,14 +111,22 @@ export async function collectFiles(ctx: PipelineContext): Promise { } ctx.allFiles = existing.map((e) => e.file); ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file))); - ctx.parseChanges = existing; - ctx.metadataUpdates = []; - ctx.removed = missing; - ctx.isFullBuild = false; - info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`); - return; + } finally { + ctx.timing.collectMs = performance.now() - start; } + // Change-detection outputs — timed under detectMs for semantic parity. + const detectStart = performance.now(); + ctx.parseChanges = existing; + ctx.metadataUpdates = []; + ctx.removed = missing; + ctx.isFullBuild = false; + ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - detectStart); + info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`); + return; + } + const start = performance.now(); + try { // Incremental fast path: reconstruct file list from DB + journal deltas // instead of full recursive filesystem scan (~8ms savings on 473 files). if (ctx.incremental && !ctx.forceFullRebuild) { diff --git a/src/domain/graph/builder/stages/detect-changes.ts b/src/domain/graph/builder/stages/detect-changes.ts index dfd4ad5b..4db72865 100644 --- a/src/domain/graph/builder/stages/detect-changes.ts +++ b/src/domain/graph/builder/stages/detect-changes.ts @@ -571,6 +571,8 @@ export async function detectChanges(ctx: PipelineContext): Promise { handleIncrementalBuild(ctx); } } finally { - ctx.timing.detectMs = performance.now() - start; + // Additive to respect any partial detectMs contribution from collectFiles + // (scoped-rebuild path splits change-detection outputs across both stages). + ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - start); } } diff --git a/tests/builder/pipeline.test.ts b/tests/builder/pipeline.test.ts index a3aee31a..c305c08d 100644 --- a/tests/builder/pipeline.test.ts +++ b/tests/builder/pipeline.test.ts @@ -32,7 +32,9 @@ describe('buildGraph pipeline', () => { expect(result.phases).toBeDefined(); expect(typeof result.phases.setupMs).toBe('number'); expect(typeof result.phases.collectMs).toBe('number'); + expect(result.phases.collectMs).toBeGreaterThanOrEqual(0); expect(typeof result.phases.detectMs).toBe('number'); + expect(result.phases.detectMs).toBeGreaterThanOrEqual(0); expect(typeof result.phases.parseMs).toBe('number'); expect(typeof result.phases.insertMs).toBe('number'); expect(typeof result.phases.resolveMs).toBe('number');