Skip to content

Commit 87ce95d

Browse files
committed
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
1 parent c2fa0d7 commit 87ce95d

3 files changed

Lines changed: 32 additions & 15 deletions

File tree

src/domain/graph/builder/stages/collect-files.ts

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,20 @@ function tryFastCollect(
8787
}
8888

8989
export async function collectFiles(ctx: PipelineContext): Promise<void> {
90-
const start = performance.now();
91-
try {
92-
const { rootDir, config, opts } = ctx;
90+
const { rootDir, config, opts } = ctx;
9391

94-
if (opts.scope) {
95-
// Scoped rebuild: rebuild only specified files
96-
const scopedFiles = opts.scope.map((f: string) => normalizePath(f));
97-
const existing: Array<{ file: string; relPath: string }> = [];
98-
const missing: string[] = [];
92+
if (opts.scope) {
93+
// Scoped rebuild: rebuild only specified files.
94+
//
95+
// Timer only wraps the filesystem-walk portion (existence checks + file
96+
// list construction). Change-detection outputs (parseChanges, removed,
97+
// isFullBuild) are attributed to detectMs for semantic consistency with
98+
// the non-scoped path, even though this stage computes them.
99+
const start = performance.now();
100+
const scopedFiles = opts.scope.map((f: string) => normalizePath(f));
101+
const existing: Array<{ file: string; relPath: string }> = [];
102+
const missing: string[] = [];
103+
try {
99104
for (const rel of scopedFiles) {
100105
const abs = path.join(rootDir, rel);
101106
if (fs.existsSync(abs)) {
@@ -106,14 +111,22 @@ export async function collectFiles(ctx: PipelineContext): Promise<void> {
106111
}
107112
ctx.allFiles = existing.map((e) => e.file);
108113
ctx.discoveredDirs = new Set(existing.map((e) => path.dirname(e.file)));
109-
ctx.parseChanges = existing;
110-
ctx.metadataUpdates = [];
111-
ctx.removed = missing;
112-
ctx.isFullBuild = false;
113-
info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`);
114-
return;
114+
} finally {
115+
ctx.timing.collectMs = performance.now() - start;
115116
}
117+
// Change-detection outputs — timed under detectMs for semantic parity.
118+
const detectStart = performance.now();
119+
ctx.parseChanges = existing;
120+
ctx.metadataUpdates = [];
121+
ctx.removed = missing;
122+
ctx.isFullBuild = false;
123+
ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - detectStart);
124+
info(`Scoped rebuild: ${existing.length} files to rebuild, ${missing.length} to purge`);
125+
return;
126+
}
116127

128+
const start = performance.now();
129+
try {
117130
// Incremental fast path: reconstruct file list from DB + journal deltas
118131
// instead of full recursive filesystem scan (~8ms savings on 473 files).
119132
if (ctx.incremental && !ctx.forceFullRebuild) {

src/domain/graph/builder/stages/detect-changes.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,8 @@ export async function detectChanges(ctx: PipelineContext): Promise<void> {
571571
handleIncrementalBuild(ctx);
572572
}
573573
} finally {
574-
ctx.timing.detectMs = performance.now() - start;
574+
// Additive to respect any partial detectMs contribution from collectFiles
575+
// (scoped-rebuild path splits change-detection outputs across both stages).
576+
ctx.timing.detectMs = (ctx.timing.detectMs ?? 0) + (performance.now() - start);
575577
}
576578
}

tests/builder/pipeline.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,9 @@ describe('buildGraph pipeline', () => {
3232
expect(result.phases).toBeDefined();
3333
expect(typeof result.phases.setupMs).toBe('number');
3434
expect(typeof result.phases.collectMs).toBe('number');
35+
expect(result.phases.collectMs).toBeGreaterThanOrEqual(0);
3536
expect(typeof result.phases.detectMs).toBe('number');
37+
expect(result.phases.detectMs).toBeGreaterThanOrEqual(0);
3638
expect(typeof result.phases.parseMs).toBe('number');
3739
expect(typeof result.phases.insertMs).toBe('number');
3840
expect(typeof result.phases.resolveMs).toBe('number');

0 commit comments

Comments
 (0)