refactor: adopt timeMedian in remaining benchmark timing loops#1905
refactor: adopt timeMedian in remaining benchmark timing loops#1905carlos-alm wants to merge 1 commit into
Conversation
Deduplicates the "loop N times, time each run, compute median" pattern
across the 7 remaining call sites that predated scripts/lib/bench-timing.ts:
- query-benchmark.ts's benchDepths()/benchDiffImpact() and
incremental-benchmark.ts's parent-process nativeBatchMs/jsFallbackMs
computations and benchmark.ts's benchQuery() now adopt timeMedian()
directly (converted to async; propagation stops at each call site
since all callers are already at ESM top-level/top-level-await scope).
- incremental-benchmark.ts's and benchmark.ts's "1-file rebuild"
measurements need the phases of the median-timed run, not just the
numeric median, so bench-timing.ts gains timeMedianWithValue(fn, runs,
beforeEach?) — returns the {ms, value} pair for the median-duration
run, with an optional untimed beforeEach(i) hook for per-iteration
setup (writing the probe file) that must stay outside the timed
window.
Verified no methodology regression: ran query-benchmark.ts and
incremental-benchmark.ts before/after on this repo (3 runs and 1 run
respectively) with consistent latencies, plus an isolated microbenchmark
quantifying the await-on-sync-value overhead at ~1 microsecond/call —
negligible against the millisecond-to-hundred-millisecond latencies
these scripts measure.
Fixes #1774
Impact: 4 functions changed, 4 affected
Greptile SummaryThis PR refactors four benchmark scripts to eliminate duplicated timing loops by adopting the shared
Confidence Score: 4/5The refactor is a clean mechanical substitution — all timing methodology is preserved and the new timeMedianWithValue helper is straightforward. The two observations are minor and non-blocking. All four files convert correctly from manual timing loops to the shared helpers. The beforeEach hook correctly excludes probe-file I/O from measured time. The await precedence issue with property access is handled correctly at every call site. The two notes flagged are both non-blocking: timeMedianWithValue returning undefined for runs=0 is not reachable at current call sites, and the lastResult mismatch in benchDiffImpact is a pre-existing semantic quirk that deterministic query results make harmless. scripts/lib/bench-timing.ts (missing empty-samples guard in timeMedianWithValue) and scripts/query-benchmark.ts (benchDiffImpact lastResult closure). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["timeMedian(fn, runs)"] --> B["Loop runs times\nawait fn\ncollect ms[]"]
B --> C["median(ms[])"]
C --> D["return number"]
E["timeMedianWithValue(fn, runs, beforeEach?)"] --> F["Loop runs times\nbeforeEach(i) outside timer\nawait fn(i) inside timer\ncollect {ms, value}[]"]
F --> G["sort by ms"]
G --> H["samples[floor(N/2)]"]
H --> I["return {ms, value}"]
J["benchmark.ts 1-file rebuild"] -->|needs phases| E
K["incremental-benchmark.ts 1-file rebuild"] -->|needs phases| E
L["benchmark.ts benchQuery"] -->|scalar only| A
M["incremental-benchmark.ts nativeBatchMs/jsFallbackMs"] -->|scalar only| A
N["query-benchmark.ts benchDepths/benchDiffImpact"] -->|scalar only| A
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["timeMedian(fn, runs)"] --> B["Loop runs times\nawait fn\ncollect ms[]"]
B --> C["median(ms[])"]
C --> D["return number"]
E["timeMedianWithValue(fn, runs, beforeEach?)"] --> F["Loop runs times\nbeforeEach(i) outside timer\nawait fn(i) inside timer\ncollect {ms, value}[]"]
F --> G["sort by ms"]
G --> H["samples[floor(N/2)]"]
H --> I["return {ms, value}"]
J["benchmark.ts 1-file rebuild"] -->|needs phases| E
K["incremental-benchmark.ts 1-file rebuild"] -->|needs phases| E
L["benchmark.ts benchQuery"] -->|scalar only| A
M["incremental-benchmark.ts nativeBatchMs/jsFallbackMs"] -->|scalar only| A
N["query-benchmark.ts benchDepths/benchDiffImpact"] -->|scalar only| A
Reviews (1): Last reviewed commit: "refactor: adopt timeMedian in remaining ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| samples.sort((a, b) => a.ms - b.ms); | ||
| return samples[Math.floor(samples.length / 2)]; | ||
| } |
There was a problem hiding this comment.
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.
| 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, |
There was a problem hiding this comment.
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!
Codegraph Impact Analysis4 functions changed → 4 callers affected across 3 files
|
1 participant
Summary
query-benchmark.ts'sbenchDepths/benchDiffImpact,incremental-benchmark.ts's parent-processnativeBatchMs/jsFallbackMs,benchmark.ts'sbenchQuery): converted to async, adoptedtimeMediandirectly. Handled anawait foo().barprecedence gotcha (parses asawait (foo().bar), not(await foo()).bar) by adding explicit parens at all affected call sites.{ms, phases}, not just a numeric median): addedtimeMedianWithValue<T>(fn, runs, beforeEach?)tobench-timing.ts. ThebeforeEachhook preserves a real pre-existing methodology detail — the probe-file write happens before timing starts in the original code; folding it into the timed closure would have silently counted disk-write time as rebuild latency.Closes #1774
Test plan
npm test— full suite green (3564 passed, 0 failed)npm run lint— clean (scripts/confirmed outside Biome's scope)node --experimental-strip-types --checkon all 4 files (scripts/ is excluded from tsconfig, so this is the actual syntax-validation mechanism these scripts rely on)await fn()overhead found ~0.7μs/call, ~4 orders of magnitude below the smallest real latency in scopeFiled #1904 for an out-of-scope finding:
query-benchmark.ts's hub-selection queries can non-deterministically resolve to any of 7 different nodes namedbuildGraph, undermining cross-run comparability.Stacked on #1902 (base branch
fix/issue-1773-destructured-binding-kind) — only the scripts/ diff is this issue's change.