From cd9be105c366a9d92226a4ce7e85012caa105310 Mon Sep 17 00:00:00 2001 From: Carlos Almeida Date: Wed, 19 Aug 2026 13:47:02 -0600 Subject: [PATCH 1/2] fix(bench): give query-benchmark.ts's diffImpact metric a warmup too WARMUP_RUNS in scripts/query-benchmark.ts was applied only inside benchDepths, which serves fnDeps and fnImpact. benchDiffImpact called timeMedian directly with no warmup, making diffImpact latency the one query metric measured cold -- the same defect class #2584 just fixed for benchmark.ts's Full build metric, and that #2436 traced part of the pre-publish gate's non-determinism to. The warmup those earlier calls already paid does not carry over: diffImpactData is a distinct query path with its own prepared statements and its own DB pages, and it shells out to git to read the staged diff. Measured on this repo, 3 runs per variant, native engine: before 10.9 / 10.8 / 11.2 ms after 9.9 / 9.8 / 9.9 ms Both a ~10% drop and a much tighter spread. WASM is unchanged (9.4/9.2/9.6 -> 9.4/9.4/9.7), as expected -- the cold-start cost this removes is the native NAPI/rusqlite one. Refs #2590 Impact: 1 functions changed, 1 affected --- scripts/query-benchmark.ts | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/scripts/query-benchmark.ts b/scripts/query-benchmark.ts index 9f8c7eaa1..b15816aee 100644 --- a/scripts/query-benchmark.ts +++ b/scripts/query-benchmark.ts @@ -109,13 +109,18 @@ console.log = (...args) => console.error(...args); const RUNS = 5; -// First 2-3 native fnDeps calls per process pay a cold-start cost (rusqlite -// statement-cache warmup, OS page cache for the DB file, NAPI-side static -// init from tree-sitter's transitive crates linked into the .node binary). -// On Linux x86_64 CI, that pulled median(5) into cold-start territory once -// tree-sitter 0.25 grew the binary's init footprint (#1076), even though -// steady-state per-call latency is unchanged. Discard the first WARMUP_RUNS -// before timing so the metric reflects warm-call latency, not cold-start. +// The first 2-3 native calls into a given query path per process pay a +// cold-start cost (rusqlite statement-cache warmup, OS page cache for the DB +// file, NAPI-side static init from tree-sitter's transitive crates linked +// into the .node binary). On Linux x86_64 CI, that pulled median(5) into +// cold-start territory once tree-sitter 0.25 grew the binary's init footprint +// (#1076), even though steady-state per-call latency is unchanged. Discard +// the first WARMUP_RUNS before timing so the metric reflects warm-call +// latency, not cold-start. +// +// This applies per query path, not just once per process: each of fnDeps, +// fnImpact and diffImpact prepares its own statements and touches its own DB +// pages, so warming one does not warm the next (#2590). const WARMUP_RUNS = 3; async function benchDepths(fn, name, depths) { @@ -166,6 +171,17 @@ async function benchDiffImpact(targets: HubTargets) { fs.writeFileSync(hubFile, original + '\n// benchmark-probe\n'); execFileSync('git', ['add', hubFile], { cwd: root, stdio: 'pipe' }); + // Warm the diffImpact path before timing, exactly as benchDepths does for + // fnDeps/fnImpact. diffImpactData is a distinct query path from those -- + // its own prepared statements, its own DB pages, and a `git` subprocess to + // read the staged diff -- so the warmup those calls already paid does not + // carry over. Without this, diffImpact was the only query metric measured + // cold: the same defect #2584 fixed for benchmark.ts's Full build, and + // that #2436 traced part of the gate's non-determinism to (#2590). + for (let i = 0; i < WARMUP_RUNS; i++) { + diffImpactData(dbPath, { staged: true, depth: 3, noTests: true }); + } + let lastResult = null; const latencyMs = round1( await timeMedian(() => { From 45b5b531e03dd61135413be4165ee7c9efff32e8 Mon Sep 17 00:00:00 2001 From: Carlos Almeida Date: Thu, 20 Aug 2026 12:19:11 -0600 Subject: [PATCH 2/2] docs(bench): correct what the diffImpact warmup actually warms (#2591) diffImpactData opens its own readonly better-sqlite3 handle and closes it in a finally, so the statement cache is connection-scoped and never survives into the timed runs. Describe the state that does persist -- loadConfig's per-root cache, the OS page cache for the DB file and .git data, and V8 tiering of this path's diff-parsing and BFS code -- instead of claiming a prepared-statement warmup that cannot happen here. --- scripts/query-benchmark.ts | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/scripts/query-benchmark.ts b/scripts/query-benchmark.ts index b15816aee..83097783e 100644 --- a/scripts/query-benchmark.ts +++ b/scripts/query-benchmark.ts @@ -118,9 +118,12 @@ const RUNS = 5; // the first WARMUP_RUNS before timing so the metric reflects warm-call // latency, not cold-start. // -// This applies per query path, not just once per process: each of fnDeps, -// fnImpact and diffImpact prepares its own statements and touches its own DB -// pages, so warming one does not warm the next (#2590). +// This applies per query path, not just once per process: fnDeps, fnImpact and +// diffImpact each run different code over different DB pages, so warming one +// does not warm the next (#2590). Which specific cold-start costs a warmup +// removes differs by path, though -- the list above describes the native +// fnDeps/fnImpact path; see benchDiffImpact for the JS/better-sqlite3 case, +// where the statement cache does not outlive a single call. const WARMUP_RUNS = 3; async function benchDepths(fn, name, depths) { @@ -172,12 +175,23 @@ async function benchDiffImpact(targets: HubTargets) { execFileSync('git', ['add', hubFile], { cwd: root, stdio: 'pipe' }); // Warm the diffImpact path before timing, exactly as benchDepths does for - // fnDeps/fnImpact. diffImpactData is a distinct query path from those -- - // its own prepared statements, its own DB pages, and a `git` subprocess to - // read the staged diff -- so the warmup those calls already paid does not - // carry over. Without this, diffImpact was the only query metric measured - // cold: the same defect #2584 fixed for benchmark.ts's Full build, and - // that #2436 traced part of the gate's non-determinism to (#2590). + // fnDeps/fnImpact. diffImpactData is a distinct query path from those -- its + // own DB pages, and a `git` subprocess to read the staged diff -- so the + // warmup those calls already paid does not carry over. + // + // What these calls warm is narrower than on the native fnDeps path, and it + // is deliberately not the statement cache: diffImpactData opens its own + // readonly better-sqlite3 handle and closes it in a `finally`, so its + // prepared statements are discarded per call and no timed run ever reuses + // one. What does carry into the timed runs is process- and OS-wide state: + // loadConfig's per-root cache (resolveDbConfig re-reads .codegraphrc.json + // only on the first call), the OS page cache for the DB file and for the + // `.git` data the staged-diff subprocess reads, and V8 tiering up the diff + // parsing and BFS code that only this path exercises. + // + // Without this, diffImpact was the only query metric measured cold: the + // same defect #2584 fixed for benchmark.ts's Full build, and that #2436 + // traced part of the gate's non-determinism to (#2590). for (let i = 0; i < WARMUP_RUNS; i++) { diffImpactData(dbPath, { staged: true, depth: 3, noTests: true }); }