Skip to content

perf(bench): exclude resolution fixtures from incremental-benchmark sweep#1131

Open
carlos-alm wants to merge 1 commit into
mainfrom
perf/1112-exclude-benchmark-fixtures
Open

perf(bench): exclude resolution fixtures from incremental-benchmark sweep#1131
carlos-alm wants to merge 1 commit into
mainfrom
perf/1112-exclude-benchmark-fixtures

Conversation

@carlos-alm
Copy link
Copy Markdown
Contributor

Summary

  • Add an optional exclude glob array to BuildGraphOpts, merged into config.exclude in setupPipeline so both the JS pipeline and the native orchestrator (which receives ctx.config as JSON) honor it.
  • Pass tests/benchmarks/resolution/fixtures/** from scripts/incremental-benchmark.ts so future language PRs don't silently inflate fullBuildMs purely from fixture parsing (e.g. tree-sitter-verilog added ~850ms / +43% on 1959 → 2809ms in feat(native): port Verilog extractor to Rust #1107).
  • Remove the 3.10.0:Full build entry (and its docstring paragraph) from KNOWN_REGRESSIONS now that the root cause is addressed.

Smoke-tested locally against this worktree (WASM engine):

without exclude — verilog nodes: 25    (19061 total nodes)
with exclude    — verilog nodes:  0    (17758 total nodes, all fixtures filtered)

Closes #1112

Test plan

  • npm run typecheck clean
  • npm run lint clean on changed files
  • RUN_REGRESSION_GUARD=1 npm test -- tests/benchmarks/regression-guard.test.ts → 17/17 pass after removing the exemption (committed 3.10.0 entry already had a clean 1959ms baseline; only the rolling dev comparison needed the exemption while fixtures inflated the number)
  • End-to-end verified: with exclude: ['tests/benchmarks/resolution/fixtures/**'], no fixture nodes land in the DB; without it, 25 verilog nodes are present
  • CI confirms the per-PR dev → 3.10.0 Full build comparison no longer hits the threshold

…weep

The incremental benchmark walks the repo root, which pulls hand-annotated
resolution-benchmark fixtures into the corpus. A single heavy grammar
(tree-sitter-verilog #1107) added ~850ms to fullBuildMs — the cost is
real but unrelated to shared code paths the benchmark is meant to track.

Add an optional exclude glob array to BuildGraphOpts, merged into
config.exclude in setupPipeline so both the JS pipeline and the native
orchestrator (which receives ctx.config as JSON) honor it. Pass
'tests/benchmarks/resolution/fixtures/**' from the benchmark script so
future language PRs don't silently inflate timings, and remove the
3.10.0:Full build entry from KNOWN_REGRESSIONS now that the cause is
addressed.

Closes #1112
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR adds an optional exclude glob array to BuildGraphOpts that gets merged into ctx.config.exclude during setupPipeline, then uses it from the incremental benchmark script to filter out tests/benchmarks/resolution/fixtures/**. The change also removes the now-obsolete 3.10.0:Full build exemption from KNOWN_REGRESSIONS.

  • src/types.ts + src/domain/graph/builder/pipeline.ts: New exclude?: string[] option on BuildGraphOpts is spliced into ctx.config.exclude before initializeEngine is called, so both the JS pipeline and the native orchestrator (which receives ctx.config as JSON) honour the extra globs.
  • scripts/incremental-benchmark.ts: Centralises all buildGraph calls through a BUILD_OPTS constant carrying BENCH_EXCLUDE, ensuring the fixture directory is excluded from every benchmark tier (full build, no-op, 1-file) uniformly.
  • tests/benchmarks/regression-guard.test.ts: Drops the 3.10.0:Full build known-regression entry whose root cause is now fixed.

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to benchmark infrastructure and a non-breaking additive field on BuildGraphOpts.

The exclude merging in pipeline.ts is applied before initializeEngine, so both execution paths see the updated list. CodegraphConfig.exclude is typed as a required string[] and always initialised from DEFAULTS, so the spread is safe. The config cache returns structuredClone'd copies, so benchmark runs don't pollute the cache. All buildGraph call sites in the benchmark are updated consistently via BUILD_OPTS. The KNOWN_REGRESSIONS cleanup is backed by local smoke-test evidence and aligns with the fix.

No files require special attention.

Important Files Changed

Filename Overview
scripts/incremental-benchmark.ts Introduces BENCH_EXCLUDE and BUILD_OPTS constants to pass the fixtures glob to every buildGraph call, consistently excluding resolution fixture files from all benchmark tiers (full, no-op, 1-file).
src/domain/graph/builder/pipeline.ts Adds opts.exclude merging into ctx.config.exclude in setupPipeline, correctly applied before initializeEngine so both JS and native orchestrator paths see the augmented exclude list.
src/types.ts Adds optional exclude?: string[] field to BuildGraphOpts with a clear JSDoc explaining the intended use case.
tests/benchmarks/regression-guard.test.ts Removes the 3.10.0:Full build exemption from KNOWN_REGRESSIONS now that the root cause (fixture files inflating fullBuildMs) is addressed by the exclude mechanism.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["buildGraph(root, opts)"] --> B["setupPipeline(ctx)"]
    B --> C["ctx.config = loadConfig(rootDir)"]
    C --> D{"opts.exclude\npresent?"}
    D -- Yes --> E["ctx.config.exclude = [...config.exclude, ...opts.exclude]"]
    D -- No --> F["ctx.config unchanged"]
    E --> G["initializeEngine(ctx)"]
    F --> G
    G --> H{"native\norchestrator?"}
    H -- Yes --> I["Rust orchestrator\nreceives ctx.config as JSON\n(honours merged exclude)"]
    H -- No --> J["JS pipeline\n(honours merged exclude)"]
    I --> K["File collection skips\nexcluded globs"]
    J --> K

    subgraph "incremental-benchmark.ts"
        L["BENCH_EXCLUDE = ['tests/benchmarks/resolution/fixtures/**']"]
        M["BUILD_OPTS = { engine, exclude: BENCH_EXCLUDE }"]
        N["buildGraph(root, { ...BUILD_OPTS, incremental: false })"]
        O["buildGraph(root, { ...BUILD_OPTS, incremental: true })"]
        L --> M
        M --> N
        M --> O
    end
Loading

Reviews (1): Last reviewed commit: "perf(bench): exclude resolution fixtures..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf: exclude verilog benchmark fixtures from incremental benchmark sweep

1 participant