Skip to content

refactor: adopt timeMedian in remaining benchmark timing loops #1905

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.

Sign up for GitHub

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

Closed
carlos-alm wants to merge 1 commit into from
Closed

refactor: adopt timeMedian in remaining benchmark timing loops #1905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 23 additions & 24 deletions scripts/benchmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { fileURLToPath } from 'node:url';
import Database from 'better-sqlite3';
import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js';
import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js';
import { median, round1, timeMedian } from './lib/bench-timing.js';
import { round1, timeMedian, timeMedianWithValue } from './lib/bench-timing.js';

// ── Parent process: fork one child per engine, assemble final output ─────
if (!isWorker()) {
Expand Down Expand Up @@ -134,7 +134,7 @@ const buildTimeMs = performance.now() - buildStart;
// query latency with NAPI/rusqlite/OS-page-cache init costs (~65ms on
// macOS) and inflated growth from test-fixture files pulled in by new
// native extractors. See #1113 for the methodology rationale.
const queryTimeMs = benchQuery(fnDepsData, 'buildGraph', dbPath, { depth: 3, noTests: true });
const queryTimeMs = await benchQuery(fnDepsData, 'buildGraph', dbPath, { depth: 3, noTests: true });

const stats = statsData(dbPath);
const totalFiles = stats.files.total;
Expand Down Expand Up @@ -168,17 +168,16 @@ try {
fs.writeFileSync(PROBE_FILE, original + `\n// warmup-${i}\n`);
await buildGraph(root, { engine, incremental: true, exclude: BENCH_EXCLUDE });
}
const oneFileRuns = [];
for (let i = 0; i < INCREMENTAL_RUNS; i++) {
fs.writeFileSync(PROBE_FILE, original + `\n// probe-${i}\n`);
const start = performance.now();
const res = await buildGraph(root, { engine, incremental: true, exclude: BENCH_EXCLUDE });
oneFileRuns.push({ ms: performance.now() - start, phases: res?.phases || null });
}
oneFileRuns.sort((a, b) => a.ms - b.ms);
const medianRun = oneFileRuns[Math.floor(oneFileRuns.length / 2)];
const medianRun = await timeMedianWithValue(
async () => {
const res = await buildGraph(root, { engine, incremental: true, exclude: BENCH_EXCLUDE });
return res?.phases || null;
},
INCREMENTAL_RUNS,
(i) => fs.writeFileSync(PROBE_FILE, original + `\n// probe-${i}\n`),
);
oneFileRebuildMs = Math.round(medianRun.ms);
oneFilePhases = medianRun.phases;
oneFilePhases = medianRun.value;
} catch (err) {
console.error(` [${engine}] 1-file rebuild failed: ${(err as Error).message}`);
} finally {
Expand All @@ -195,26 +194,26 @@ console.error(` [${engine}] Benchmarking queries...`);
const targets = workerTargets() || selectTargets();
console.error(` hub=${targets.hub}, leaf=${targets.leaf}`);

function benchQuery(fn, ...args) {
async function benchQuery(fn, ...args) {
// Warmup runs prime NAPI bindings, the rusqlite statement cache, and the
// OS page cache so the timed loop measures steady-state query latency
// rather than first-call init (~65ms on macOS). Each call site warms
// independently — methodology does not rely on call ordering elsewhere.
for (let i = 0; i < QUERY_WARMUP_RUNS; i++) fn(...args);
const timings = [];
for (let i = 0; i < QUERY_RUNS; i++) {
const start = performance.now();
fn(...args);
timings.push(performance.now() - start);
}
return round1(median(timings));
return round1(await timeMedian(() => fn(...args), QUERY_RUNS));
}

const queries = {
fnDepsMs: fnDepsData ? benchQuery(fnDepsData, targets.hub, dbPath, { depth: 3, noTests: true }) : null,
fnImpactMs: fnImpactData ? benchQuery(fnImpactData, targets.hub, dbPath, { depth: 3, noTests: true }) : null,
pathMs: pathData ? benchQuery(pathData, targets.hub, targets.leaf, dbPath, { noTests: true }) : null,
rolesMs: rolesData ? benchQuery(rolesData, dbPath, { noTests: true }) : null,
fnDepsMs: fnDepsData
? await benchQuery(fnDepsData, targets.hub, dbPath, { depth: 3, noTests: true })
: null,
fnImpactMs: fnImpactData
? await benchQuery(fnImpactData, targets.hub, dbPath, { depth: 3, noTests: true })
: null,
pathMs: pathData
? await benchQuery(pathData, targets.hub, targets.leaf, dbPath, { noTests: true })
: null,
rolesMs: rolesData ? await benchQuery(rolesData, dbPath, { noTests: true }) : null,
};

// Restore console.log for JSON output
Expand Down
45 changes: 18 additions & 27 deletions scripts/incremental-benchmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { performance } from 'node:perf_hooks';
import { fileURLToPath } from 'node:url';
import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js';
import { isWorker, workerEngine, forkEngines } from './lib/fork-engine.js';
import { median, round1, timeMedian } from './lib/bench-timing.js';
import { round1, timeMedian, timeMedianWithValue } from './lib/bench-timing.js';

// ── Parent process: fork one child per engine, assemble final output ─────
if (!isWorker()) {
Expand Down Expand Up @@ -87,29 +87,21 @@ if (!isWorker()) {
for (let i = 0; i < WARMUP_RUNS; i++) {
parentBatch(inputs, rootParent, null);
}
const timings = [];
for (let i = 0; i < RUNS; i++) {
const start = performance.now();
parentBatch(inputs, rootParent, null);
timings.push(performance.now() - start);
}
nativeBatchMs = round1(median(timings));
nativeBatchMs = round1(await timeMedian(() => parentBatch(inputs, rootParent, null), RUNS));
perImportNativeMs = inputs.length > 0 ? round1(nativeBatchMs / inputs.length) : 0;
}
for (let i = 0; i < WARMUP_RUNS; i++) {
for (const { fromFile, importSource } of inputs) {
parentJS(fromFile, importSource, rootParent, null);
}
}
const jsTimings = [];
for (let i = 0; i < RUNS; i++) {
const start = performance.now();
for (const { fromFile, importSource } of inputs) {
parentJS(fromFile, importSource, rootParent, null);
}
jsTimings.push(performance.now() - start);
}
const jsFallbackMs = round1(median(jsTimings));
const jsFallbackMs = round1(
await timeMedian(() => {
for (const { fromFile, importSource } of inputs) {
parentJS(fromFile, importSource, rootParent, null);
}
}, RUNS),
);
const perImportJsMs = inputs.length > 0 ? round1(jsFallbackMs / inputs.length) : 0;

const resolve = { imports: inputs.length, nativeBatchMs, jsFallbackMs, perImportNativeMs, perImportJsMs };
Expand Down Expand Up @@ -217,17 +209,16 @@ try {
fs.writeFileSync(PROBE_FILE, original + `\n// warmup-${i}\n`);
await buildGraph(root, { ...BUILD_OPTS, incremental: true });
}
const oneFileRuns = [];
for (let i = 0; i < RUNS; i++) {
fs.writeFileSync(PROBE_FILE, original + `\n// probe-${i}\n`);
const start = performance.now();
const res = await buildGraph(root, { ...BUILD_OPTS, incremental: true });
oneFileRuns.push({ ms: performance.now() - start, phases: res?.phases || null });
}
oneFileRuns.sort((a, b) => a.ms - b.ms);
const medianRun = oneFileRuns[Math.floor(oneFileRuns.length / 2)];
const medianRun = await timeMedianWithValue(
async () => {
const res = await buildGraph(root, { ...BUILD_OPTS, incremental: true });
return res?.phases || null;
},
RUNS,
(i) => fs.writeFileSync(PROBE_FILE, original + `\n// probe-${i}\n`),
);
oneFileRebuildMs = Math.round(medianRun.ms);
oneFilePhases = medianRun.phases;
oneFilePhases = medianRun.value;
} catch (err) {
console.error(` [${engine}] 1-file rebuild failed: ${(err as Error).message}`);
} finally {
Expand Down
33 changes: 33 additions & 0 deletions scripts/lib/bench-timing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
* const fullBuildMs = Math.round(
* await timeMedian(() => buildGraph(root, { engine, incremental: false }), RUNS),
* );
*
* `timeMedianWithValue` covers the same loop shape for call sites that also
* need side data from whichever run turned out to be the median-duration one
* (e.g. the build-phase breakdown of a "1-file rebuild" measurement) — data
* `timeMedian`'s bare `Promise<number>` return can't carry.
*/
import { performance } from 'node:perf_hooks';

Expand Down Expand Up @@ -51,3 +56,31 @@ export async function timeMedian(fn: () => unknown, runs: number): Promise<numbe
}
return median(timings);
}

/**
* Runs `fn` `runs` times, recording elapsed milliseconds and `fn`'s return
* value per run, and returns the `{ ms, value }` pair for the median-duration
* run. An optional `beforeEach(i)` hook runs immediately before each timed
* call — its cost is *not* included in the timed duration — for per-iteration
* setup like writing a probe file with iteration-varying content ahead of an
* incremental rebuild.
*
* Unlike `median()`, this does not average the two middle samples when `runs`
* is even — it returns one real sampled run (sorted by `ms`, middle index),
* so `value` always stays attributable to an actual execution.
*/
export async function timeMedianWithValue<T>(
fn: (i: number) => T | Promise<T>,
runs: number,
beforeEach?: (i: number) => void | Promise<void>,
): Promise<{ ms: number; value: T }> {
const samples: { ms: number; value: T }[] = [];
for (let i = 0; i < runs; i++) {
if (beforeEach) await beforeEach(i);
const start = performance.now();
const value = await fn(i);
samples.push({ ms: performance.now() - start, value });
}
samples.sort((a, b) => a.ms - b.ms);
return samples[Math.floor(samples.length / 2)];
}
Comment on lines +83 to +86

@greptile-apps greptile-apps Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing empty-samples guard

If runs is ever 0, samples is empty and samples[Math.floor(0/2)] returns undefined. Any caller that then accesses .ms or .value will throw a TypeError. The sibling median() handles this with an early if (arr.length === 0) return 0 guard — timeMedianWithValue has no equivalent, making it inconsistent with the rest of the module's defensive pattern.

Fix in Claude Code

43 changes: 19 additions & 24 deletions scripts/query-benchmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { fileURLToPath } from 'node:url';
import Database from 'better-sqlite3';
import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js';
import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js';
import { median, round1 } from './lib/bench-timing.js';
import { round1, timeMedian } from './lib/bench-timing.js';

// ── Parent process: fork one child per engine, assemble final output ─────
if (!isWorker()) {
Expand Down Expand Up @@ -169,19 +169,15 @@ function selectTargets() {
}
}

function benchDepths(fn, name, depths) {
async function benchDepths(fn, name, depths) {
const result = {};
for (const depth of depths) {
for (let i = 0; i < WARMUP_RUNS; i++) {
fn(name, dbPath, { depth, noTests: true });
}
const timings = [];
for (let i = 0; i < RUNS; i++) {
const start = performance.now();
fn(name, dbPath, { depth, noTests: true });
timings.push(performance.now() - start);
}
result[`depth${depth}Ms`] = round1(median(timings));
result[`depth${depth}Ms`] = round1(
await timeMedian(() => fn(name, dbPath, { depth, noTests: true }), RUNS),
);
}
return result;
}
Expand All @@ -201,7 +197,7 @@ function resolveDbFile(rootDir: string, dbFile: string): string | null {
return null;
}

function benchDiffImpact(hubName) {
async function benchDiffImpact(hubName) {
const db = new Database(dbPath, { readonly: true });
const row = db
.prepare(`SELECT file FROM nodes WHERE name = ? LIMIT 1`)
Expand All @@ -224,16 +220,15 @@ function benchDiffImpact(hubName) {
fs.writeFileSync(hubFile, original + '\n// benchmark-probe\n');
execFileSync('git', ['add', hubFile], { cwd: root, stdio: 'pipe' });

const timings = [];
let lastResult = null;
for (let i = 0; i < RUNS; i++) {
const start = performance.now();
lastResult = diffImpactData(dbPath, { staged: true, depth: 3, noTests: true });
timings.push(performance.now() - start);
}
const latencyMs = round1(
await timeMedian(() => {
lastResult = diffImpactData(dbPath, { staged: true, depth: 3, noTests: true });
}, RUNS),
);

return {
latencyMs: round1(median(timings)),
latencyMs,
affectedFunctions: lastResult?.affectedFunctions?.length || 0,
affectedFiles: lastResult?.affectedFiles?.length || 0,
Comment on lines 223 to 233

@greptile-apps greptile-apps Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 lastResult reflects the last chronological run, not the median-duration run

timeMedian runs iterations sequentially from i=0 to i=RUNS-1, so after the loop lastResult always holds the result of the final iteration — not the run whose timing was selected as the median. This creates a conceptual mismatch: latencyMs comes from the median-duration run, while affectedFunctions/affectedFiles come from whichever run happened to be last. timeMedianWithValue was purpose-built for exactly this shape (scalar timing + associated side data per run), and would keep the reported count attributable to the same run whose latency is reported.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

};
Expand All @@ -253,15 +248,15 @@ console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.l
const fnDeps = {};
const fnImpact = {};

fnDeps.depth1Ms = benchDepths(fnDepsData, targets.hub, [1]).depth1Ms;
fnDeps.depth3Ms = benchDepths(fnDepsData, targets.hub, [3]).depth3Ms;
fnDeps.depth5Ms = benchDepths(fnDepsData, targets.hub, [5]).depth5Ms;
fnDeps.depth1Ms = (await benchDepths(fnDepsData, targets.hub, [1])).depth1Ms;
fnDeps.depth3Ms = (await benchDepths(fnDepsData, targets.hub, [3])).depth3Ms;
fnDeps.depth5Ms = (await benchDepths(fnDepsData, targets.hub, [5])).depth5Ms;

fnImpact.depth1Ms = benchDepths(fnImpactData, targets.hub, [1]).depth1Ms;
fnImpact.depth3Ms = benchDepths(fnImpactData, targets.hub, [3]).depth3Ms;
fnImpact.depth5Ms = benchDepths(fnImpactData, targets.hub, [5]).depth5Ms;
fnImpact.depth1Ms = (await benchDepths(fnImpactData, targets.hub, [1])).depth1Ms;
fnImpact.depth3Ms = (await benchDepths(fnImpactData, targets.hub, [3])).depth3Ms;
fnImpact.depth5Ms = (await benchDepths(fnImpactData, targets.hub, [5])).depth5Ms;

const diffImpact = benchDiffImpact(targets.hub);
const diffImpact = await benchDiffImpact(targets.hub);

// Restore console.log for JSON output
console.log = origLog;
Expand Down
Loading