diff --git a/scripts/benchmark.ts b/scripts/benchmark.ts index f8daa0ad..fbc44981 100644 --- a/scripts/benchmark.ts +++ b/scripts/benchmark.ts @@ -15,7 +15,7 @@ import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { fileURLToPath } from 'node:url'; import Database from 'better-sqlite3'; -import { resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; +import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js'; // ── Parent process: fork one child per engine, assemble final output ───── @@ -94,6 +94,7 @@ const INCREMENTAL_RUNS = 3; const QUERY_RUNS = 5; const QUERY_WARMUP_RUNS = 3; const PROBE_FILE = path.join(root, 'src', 'domain', 'queries.ts'); +const BENCH_EXCLUDE = [...resolveBenchmarkExcludes()]; function median(arr) { const sorted = [...arr].sort((a, b) => a - b); @@ -131,7 +132,7 @@ console.log = (...args) => console.error(...args); if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); const buildStart = performance.now(); -const buildResult = await buildGraph(root, { engine, incremental: false }); +const buildResult = await buildGraph(root, { engine, incremental: false, exclude: BENCH_EXCLUDE }); const buildTimeMs = performance.now() - buildStart; // Warmed median of QUERY_RUNS samples with `noTests: true` to match the @@ -156,7 +157,7 @@ try { const noopTimings = []; for (let i = 0; i < INCREMENTAL_RUNS; i++) { const start = performance.now(); - await buildGraph(root, { engine, incremental: true }); + await buildGraph(root, { engine, incremental: true, exclude: BENCH_EXCLUDE }); noopTimings.push(performance.now() - start); } noopRebuildMs = Math.round(median(noopTimings)); @@ -173,7 +174,7 @@ try { 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 }); + 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); @@ -185,7 +186,7 @@ try { } finally { fs.writeFileSync(PROBE_FILE, original); try { - await buildGraph(root, { engine, incremental: true }); + await buildGraph(root, { engine, incremental: true, exclude: BENCH_EXCLUDE }); } catch { // Cleanup rebuild failed — probe file is already restored, move on } diff --git a/scripts/incremental-benchmark.ts b/scripts/incremental-benchmark.ts index f3d19a7f..853d595d 100644 --- a/scripts/incremental-benchmark.ts +++ b/scripts/incremental-benchmark.ts @@ -14,7 +14,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { fileURLToPath } from 'node:url'; -import { resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; +import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; import { isWorker, workerEngine, forkEngines } from './lib/fork-engine.js'; // ── Parent process: fork one child per engine, assemble final output ───── @@ -181,14 +181,14 @@ const PROBE_FILE = path.join(root, 'src', 'domain', 'queries.ts'); // CI-amplified false regressions on sub-30ms metrics like No-op rebuild. const WARMUP_RUNS = 2; -// Resolution-benchmark fixtures live under the repo root and get pulled into -// every self-build sweep. They are hand-annotated test corpora — not real -// codegraph code — and a single heavy grammar (e.g. tree-sitter-verilog) can -// add hundreds of milliseconds to fullBuildMs purely from fixture parsing -// (#1112). Exclude them so adding native support for a new language doesn't -// silently inflate the incremental-benchmark numbers. -const BENCH_EXCLUDE = ['tests/benchmarks/resolution/fixtures/**']; -const BUILD_OPTS = { engine, exclude: BENCH_EXCLUDE }; +// Resolution-benchmark fixtures (`BENCHMARK_EXCLUDES` in scripts/lib/bench-config.ts) +// are excluded from every benchmark `buildGraph` call. See that constant for the +// full rationale — short version: hand-annotated fixtures aren't representative +// of real source, and heavyweight grammars (#1107) silently inflate timings. +// `resolveBenchmarkExcludes` returns `[]` in `--npm` mode so the baseline (an +// older published version that ignores `opts.exclude`) and the dev run sweep +// the same corpus. +const BUILD_OPTS = { engine, exclude: [...resolveBenchmarkExcludes()] }; function median(arr) { const sorted = [...arr].sort((a, b) => a - b); diff --git a/scripts/lib/bench-config.ts b/scripts/lib/bench-config.ts index 0ed6f784..44000d0a 100644 --- a/scripts/lib/bench-config.ts +++ b/scripts/lib/bench-config.ts @@ -15,6 +15,57 @@ import { pathToFileURL } from 'node:url'; import { getBenchmarkVersion } from '../bench-version.js'; +/** + * Globs excluded from every benchmark's `buildGraph(root, ...)` invocation. + * + * Resolution-benchmark fixtures (`tests/benchmarks/resolution/fixtures/**`) + * are hand-annotated scaffolding for the static-resolution test suite, not + * representative source code. They inflate dogfooding timing measurements + * disproportionately whenever a new-language PR lands a heavyweight grammar + * (e.g. tree-sitter-verilog added ~850ms to native fullBuildMs in #1107). + * Excluding them here keeps build/query/incremental benchmarks measuring + * codegraph's own source rather than its test fixtures. + * + * NOTE: callers should generally prefer `resolveBenchmarkExcludes()` instead + * of this constant. The helper returns `[]` in `--npm` mode so the dev-vs- + * baseline corpus stays consistent — published versions before this PR + * silently dropped `opts.exclude`, which would otherwise leave the baseline + * sweeping fixtures while the dev run skipped them. + */ +export const BENCHMARK_EXCLUDES: readonly string[] = [ + 'tests/benchmarks/resolution/fixtures/**', +]; + +/** + * `BENCHMARK_EXCLUDES` in local mode; `[]` in `--npm` mode. + * + * `--npm` benchmarks load `buildGraph` from a previously-published version + * via `srcImport(srcDir, ...)`. Releases before `BuildGraphOpts.exclude` + * landed don't recognise the option and silently drop it, so passing the + * excludes to a stale baseline would make it sweep ~745 files while the dev + * run sweeps ~607 — a corpus-mismatch that disguises measurement-shift as a + * perf delta. Emitting `[]` in `--npm` mode keeps the comparison apples-to- + * apples; the warning makes the methodology shift explicit. + * + * Idempotent across calls (the warning is printed on the first invocation + * only — `process.stderr.write` is a no-op but the helper is conceptually + * "compute once, return constant"); intentionally synchronous because + * `parseArgs` is. + */ +let warnedAboutNpmExcludeSkip = false; +export function resolveBenchmarkExcludes(): readonly string[] { + const { npm } = parseArgs(); + if (!npm) return BENCHMARK_EXCLUDES; + if (!warnedAboutNpmExcludeSkip) { + console.error( + 'Note: --npm mode skips BENCHMARK_EXCLUDES so the baseline and dev runs sweep the same corpus. ' + + 'Published versions before #1134 ignore opts.exclude silently; passing it would skew dev timings down by ~138 fewer files.', + ); + warnedAboutNpmExcludeSkip = true; + } + return []; +} + // On Windows, `npm` is `npm.cmd` and Node refuses to spawn `.cmd`/`.bat` // without `shell: true` (since Node 18.20 / 20.15). When `shell: true`, the // Windows `cmd.exe` shell resolves bare `npm` to `npm.cmd` automatically, so diff --git a/scripts/query-benchmark.ts b/scripts/query-benchmark.ts index 11c328c6..0d53d75e 100644 --- a/scripts/query-benchmark.ts +++ b/scripts/query-benchmark.ts @@ -16,7 +16,7 @@ import path from 'node:path'; import { performance } from 'node:perf_hooks'; import { fileURLToPath } from 'node:url'; import Database from 'better-sqlite3'; -import { resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; +import { resolveBenchmarkExcludes, resolveBenchmarkSource, srcImport } from './lib/bench-config.js'; import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js'; // ── Parent process: fork one child per engine, assemble final output ───── @@ -254,7 +254,7 @@ function benchDiffImpact(hubName) { // Build graph for this engine if (fs.existsSync(dbPath)) fs.unlinkSync(dbPath); -await buildGraph(root, { engine, incremental: false }); +await buildGraph(root, { engine, incremental: false, exclude: [...resolveBenchmarkExcludes()] }); const targets = workerTargets() || selectTargets(); console.error(`Targets: hub=${targets.hub}, mid=${targets.mid}, leaf=${targets.leaf}`); diff --git a/src/domain/graph/builder/pipeline.ts b/src/domain/graph/builder/pipeline.ts index c83f72d9..680544cb 100644 --- a/src/domain/graph/builder/pipeline.ts +++ b/src/domain/graph/builder/pipeline.ts @@ -185,8 +185,15 @@ function setupPipeline(ctx: PipelineContext): void { initSchema(ctx.db); ctx.config = loadConfig(ctx.rootDir); - if (ctx.opts.exclude && ctx.opts.exclude.length > 0) { - ctx.config = { ...ctx.config, exclude: [...ctx.config.exclude, ...ctx.opts.exclude] }; + // Merge caller-supplied excludes on top of the file-config excludes so + // programmatic callers (e.g. benchmark scripts) can extend exclusion + // without mutating .codegraphrc.json. Native orchestrator picks this up + // automatically — it reads exclude off the serialized ctx.config below. + if (ctx.opts.exclude?.length) { + ctx.config = { + ...ctx.config, + exclude: [...(ctx.config.exclude ?? []), ...ctx.opts.exclude], + }; } ctx.incremental = ctx.opts.incremental !== false && ctx.config.build && ctx.config.build.incremental !== false; diff --git a/src/types.ts b/src/types.ts index 6d8f31b7..8e4bf13c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1063,13 +1063,14 @@ export interface BuildGraphOpts { complexity?: boolean; cfg?: boolean; scope?: string[]; - skipRegistry?: boolean; /** - * Extra exclude globs appended to `config.exclude`. Lets benchmark scripts - * skip fixture directories that bloat self-build timings without permanently - * affecting `.codegraphrc.json` for normal users. + * Glob patterns merged on top of `config.exclude` for this build only. + * Lets callers extend exclusion programmatically without writing a config + * file — used by the benchmark scripts to skip resolution-benchmark + * fixtures that aren't representative of real code. */ exclude?: string[]; + skipRegistry?: boolean; } /** Build timing result from buildGraph. */ diff --git a/tests/benchmarks/regression-guard.test.ts b/tests/benchmarks/regression-guard.test.ts index 54b3f5ae..c376d8bf 100644 --- a/tests/benchmarks/regression-guard.test.ts +++ b/tests/benchmarks/regression-guard.test.ts @@ -209,6 +209,20 @@ const SKIP_VERSIONS = new Set(['3.8.0']); * (24.3 → 45.6ms) is consistent with the other depths. Tracked in #1113 * alongside depth 1, depth 5, and Query time; remove all four once * 3.11.0+ data confirms the new steady-state. + * + * - 3.10.0:DB bytes/file — one-time per-file methodology shift introduced + * by #1134, which excludes `tests/benchmarks/resolution/fixtures/**` + * from the dogfooding `buildGraph` sweep so heavyweight new grammars + * (e.g. Verilog #1107) no longer inflate timing. The 3.10.0 baseline + * was measured with fixtures in the corpus (~745 files); dev now + * measures the codegraph source alone (~607 files). DB content is + * dominated by `src/`, so total bytes stay roughly constant while the + * denominator drops, inflating `dbSizeBytes/file` from 41614 → ~52211 + * (+25%, exactly at the threshold). This is the same shape as the old + * `3.10.0:Full build` exemption (now removed because Full build absolute + * timing actually improved) — a measurement-scope artifact, not a real + * regression in the schema or extraction layer. Exempt this release; + * remove once 3.11.0+ data is captured under the post-#1134 methodology. */ const KNOWN_REGRESSIONS = new Set([ '3.9.6:Build ms/file', @@ -222,6 +236,7 @@ const KNOWN_REGRESSIONS = new Set([ '3.10.0:fnDeps depth 3', '3.10.0:fnDeps depth 5', '3.10.0:Query time', + '3.10.0:DB bytes/file', ]); /** diff --git a/tests/integration/config-include-exclude.test.ts b/tests/integration/config-include-exclude.test.ts index dd3655ee..83839459 100644 --- a/tests/integration/config-include-exclude.test.ts +++ b/tests/integration/config-include-exclude.test.ts @@ -178,4 +178,103 @@ describe('config.include / config.exclude (issue #981)', () => { // Paths are already relative to each run's own tmpDir so they compare directly. expect(nativeFiles).toEqual(wasmFiles); }); + + // ── opts.exclude (programmatic, no on-disk config) ─────────────── + + async function buildWithOptsExclude( + root: string, + engine: EngineName, + optsExclude: string[], + ): Promise { + clearConfigCache(); + const dbDir = path.join(root, '.codegraph'); + if (fs.existsSync(dbDir)) fs.rmSync(dbDir, { recursive: true, force: true }); + await buildGraph(root, { engine, exclude: optsExclude, skipRegistry: true }); + const files = readFileRows(path.join(dbDir, 'graph.db')); + return files.map((f) => f.replace(/\\/g, '/')).sort(); + } + + it('wasm: opts.exclude rejects matching files without writing config', async () => { + const root = fs.mkdtempSync(path.join(tmpDir, 'opts-wasm-')); + writeFixture(root); + const files = await buildWithOptsExclude(root, 'wasm', ['**/*.test.js', '**/*.spec.js']); + expect(files).toContain('src/math.js'); + expect(files).not.toContain('src/math.test.js'); + expect(files).not.toContain('src/util.spec.js'); + }); + + itNative('native: opts.exclude rejects matching files without writing config', async () => { + const root = fs.mkdtempSync(path.join(tmpDir, 'opts-native-')); + writeFixture(root); + const files = await buildWithOptsExclude(root, 'native', ['**/*.test.js', '**/*.spec.js']); + expect(files).toContain('src/math.js'); + expect(files).not.toContain('src/math.test.js'); + expect(files).not.toContain('src/util.spec.js'); + }); + + // ── opts.exclude incremental round trip ────────────────────────── + // + // Greptile feedback on PR #1134: the opts.exclude tests above always wipe + // the DB before building, so they only exercise the fresh-build path. The + // scenario where files that were previously indexed become excluded on a + // subsequent incremental run (i.e. opts.exclude changes between builds + // against the same DB) was untested. This round trip locks in the + // collect → detect behaviour: the second build must observe the newly + // excluded files as removals and drop them from file_hashes. + + async function buildSameDb( + root: string, + engine: EngineName, + optsExclude: string[] | undefined, + ): Promise { + clearConfigCache(); + await buildGraph(root, { + engine, + ...(optsExclude !== undefined ? { exclude: optsExclude } : {}), + skipRegistry: true, + }); + const files = readFileRows(path.join(root, '.codegraph', 'graph.db')); + return files.map((f) => f.replace(/\\/g, '/')).sort(); + } + + it('wasm: opts.exclude introduced on second incremental build drops previously-indexed files', async () => { + const root = fs.mkdtempSync(path.join(tmpDir, 'opts-inc-wasm-')); + writeFixture(root); + // Wipe DB so the first build is a clean baseline that indexes everything. + const dbDir = path.join(root, '.codegraph'); + if (fs.existsSync(dbDir)) fs.rmSync(dbDir, { recursive: true, force: true }); + + // First build: no exclude — every supported file is indexed. + const firstFiles = await buildSameDb(root, 'wasm', undefined); + expect(firstFiles).toContain('src/math.test.js'); + expect(firstFiles).toContain('src/util.spec.js'); + + // Second build against the same DB with exclude — previously-indexed + // test files must be detected as removals and disappear from file_hashes. + const secondFiles = await buildSameDb(root, 'wasm', ['**/*.test.js', '**/*.spec.js']); + expect(secondFiles).toContain('src/math.js'); + expect(secondFiles).toContain('src/util.js'); + expect(secondFiles).not.toContain('src/math.test.js'); + expect(secondFiles).not.toContain('src/util.spec.js'); + }); + + itNative( + 'native: opts.exclude introduced on second incremental build drops previously-indexed files', + async () => { + const root = fs.mkdtempSync(path.join(tmpDir, 'opts-inc-native-')); + writeFixture(root); + const dbDir = path.join(root, '.codegraph'); + if (fs.existsSync(dbDir)) fs.rmSync(dbDir, { recursive: true, force: true }); + + const firstFiles = await buildSameDb(root, 'native', undefined); + expect(firstFiles).toContain('src/math.test.js'); + expect(firstFiles).toContain('src/util.spec.js'); + + const secondFiles = await buildSameDb(root, 'native', ['**/*.test.js', '**/*.spec.js']); + expect(secondFiles).toContain('src/math.js'); + expect(secondFiles).toContain('src/util.js'); + expect(secondFiles).not.toContain('src/math.test.js'); + expect(secondFiles).not.toContain('src/util.spec.js'); + }, + ); });