Skip to content

feat: introduce metabench, a unified benchmarking framework - #732

Open
Martin Taillefer (geeknoid) wants to merge 1 commit into
mainfrom
metabench
Open

feat: introduce metabench, a unified benchmarking framework#732
Martin Taillefer (geeknoid) wants to merge 1 commit into
mainfrom
metabench

Conversation

@geeknoid

Copy link
Copy Markdown
Member

No description provided.

Copilot AI lite review requested due to automatic review settings September 4, 2026 15:39
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

⚠️ Manual proc-macro SemVer review required

cargo semver-checks intentionally does not analyse proc-macro-only targets. 1 of 3 publishing crate(s) require manual review either for their own procedural macro contract or because they directly consume a manually reviewed breaking release. Ordinary libraries retain their automated result, and review advances to the next dependency edge only when the current release is breaking.

Crate Baseline Baseline commit This PR Minimum required Status
metabench new crate 0.1.0 0.1.0 ✅ ok
metabench_macros new crate 0.1.0 ⚠️ manual proc-macro review required
metabench_macros_impl new crate 0.1.0 0.1.0 ✅ ok
⚠️ metabench_macros — manual proc-macro review detail
`metabench_macros` is a proc-macro-only crate. cargo-semver-checks intentionally skips proc-macro targets because they have no supported library API surface. Review exported macro names, accepted input syntax, diagnostics, and generated output manually; build and test results do not establish public API SemVer compatibility.

Proc-macro API compatibility must be reviewed manually; successful builds and tests do not establish SemVer compatibility. This check is informational and does not block the merge.

View the check run

Copilot AI left a comment

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.

🟡 Changes recommended

There are a few repo-convention and operational issues (coverage-gate test-module annotations, very deep committed fixture paths, and a more robust workflow invocation) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Introduces metabench, a new unified benchmarking framework intended to run the same workload across Criterion, Gungraun/Callgrind, Linux perf, and allocation tracking, and integrates it into the workspace + CI.

Changes:

  • Adds new metabench runtime crate plus metabench_macros / metabench_macros_impl proc-macro crates, with benches and integration tests.
  • Adds extensive fixture-based parsing tests for Criterion/Gungraun/perf/allocation artifacts and a CI smoke-check for benchmark registration output.
  • Updates workspace dependencies/lockfile and wires metabench into mutation-testing grouping, spellcheck dictionary, and Miri exclusions.
File summaries
File Description
scripts/test-metabench-registration.sh New CI script that asserts stable --list benchmark identities for metabench bench targets.
scripts/mutants.rs Adds metabench crates to cargo-mutants test grouping.
README.md Documents metabench as a primary crate in the repo.
crates/metabench/tests/report_api.rs Integration tests for report wire format + IO behavior and invariants.
crates/metabench/tests/fixtures/artifacts/traversal/too_deep/d1/d2/d3/d4/d5/d6/d7/d8/d9/d10/d11/d12/d13/d14/d15/d16/d17/d18/d19/d20/d21/d22/d23/d24/d25/d26/d27/d28/d29/d30/d31/d32/d33/d34/d35/d36/d37/d38/d39/d40/d41/d42/d43/d44/d45/d46/d47/d48/d49/d50/d51/d52/d53/d54/d55/d56/d57/d58/d59/d60/d61/d62/d63/d64/d65/.keep Depth-limit sentinel fixture for artifact traversal.
crates/metabench/tests/fixtures/artifacts/traversal/cycle/new/estimates.json Traversal fixture: Criterion estimates file for cycle/symlink scenario.
crates/metabench/tests/fixtures/artifacts/traversal/cycle/new/benchmark.json Traversal fixture: Criterion benchmark metadata for cycle/symlink scenario.
crates/metabench/tests/fixtures/artifacts/perf/unicode.jsonl Perf fixture: unicode event name JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/minimal.jsonl Perf fixture: minimal valid perf JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/malformed.jsonl Perf fixture: malformed JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/invalid_numeric.jsonl Perf fixture: invalid numeric counter value.
crates/metabench/tests/fixtures/artifacts/perf/empty.jsonl Perf fixture: empty file for “no records” error path.
crates/metabench/tests/fixtures/artifacts/perf/duplicate.jsonl Perf fixture: duplicate event records.
crates/metabench/tests/fixtures/artifacts/gungraun/wrong_mapping/summary.json Gungraun fixture: summary with out-of-range case mapping.
crates/metabench/tests/fixtures/artifacts/gungraun/minimal/summary.json Gungraun fixture: minimal valid v6 summary.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_wrong_set/metabench-identities.json Gungraun fixture: identity manifest with wrong set.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/metabench-identities.json Gungraun fixture: identity manifest for ordering test.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/b/summary.json Gungraun fixture: summary part of manifest ordering scenario.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/a/summary.json Gungraun fixture: summary part of manifest ordering scenario.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_malformed/metabench-identities.json Gungraun fixture: malformed identity manifest JSON.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_duplicate/metabench-identities.json Gungraun fixture: duplicate identities in manifest.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_count/metabench-identities.json Gungraun fixture: wrong manifest count vs selected benchmarks.
crates/metabench/tests/fixtures/artifacts/gungraun/malformed/summary.json Gungraun fixture: malformed summary JSON.
crates/metabench/tests/fixtures/artifacts/gungraun/invalid_numeric/summary.json Gungraun fixture: invalid numeric metric type.
crates/metabench/tests/fixtures/artifacts/gungraun/empty/.keep Gungraun fixture: empty directory sentinel.
crates/metabench/tests/fixtures/artifacts/gungraun/duplicate/b/summary.json Gungraun fixture: duplicate case index scenario (b).
crates/metabench/tests/fixtures/artifacts/gungraun/duplicate/a/summary.json Gungraun fixture: duplicate case index scenario (a).
crates/metabench/tests/fixtures/artifacts/criterion/unicode/new/estimates.json Criterion fixture: unicode identity estimates.
crates/metabench/tests/fixtures/artifacts/criterion/unicode/new/benchmark.json Criterion fixture: unicode identity metadata.
crates/metabench/tests/fixtures/artifacts/criterion/missing_function/new/estimates.json Criterion fixture: missing function id estimates.
crates/metabench/tests/fixtures/artifacts/criterion/missing_function/new/benchmark.json Criterion fixture: missing function id metadata.
crates/metabench/tests/fixtures/artifacts/criterion/minimal/new/estimates.json Criterion fixture: minimal valid estimates.
crates/metabench/tests/fixtures/artifacts/criterion/minimal/new/benchmark.json Criterion fixture: minimal valid metadata.
crates/metabench/tests/fixtures/artifacts/criterion/malformed/new/estimates.json Criterion fixture: malformed estimates.
crates/metabench/tests/fixtures/artifacts/criterion/malformed/new/benchmark.json Criterion fixture: malformed metadata.
crates/metabench/tests/fixtures/artifacts/criterion/invalid_numeric/new/estimates.json Criterion fixture: invalid numeric duration.
crates/metabench/tests/fixtures/artifacts/criterion/invalid_numeric/new/benchmark.json Criterion fixture: metadata paired with invalid numeric estimates.
crates/metabench/tests/fixtures/artifacts/criterion/empty/.keep Criterion fixture: empty directory sentinel.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/b/new/estimates.json Criterion fixture: duplicate identity scenario (b) estimates.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/b/new/benchmark.json Criterion fixture: duplicate identity scenario (b) metadata.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/a/new/estimates.json Criterion fixture: duplicate identity scenario (a) estimates.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/a/new/benchmark.json Criterion fixture: duplicate identity scenario (a) metadata.
crates/metabench/tests/fixtures/artifacts/allocations/minimal.json Allocation fixture: minimal valid allocation metrics.
crates/metabench/tests/fixtures/artifacts/allocations/malformed.json Allocation fixture: malformed JSON.
crates/metabench/tests/fixtures/artifacts/allocations/invalid_numeric.json Allocation fixture: invalid numeric values.
crates/metabench/tests/fixtures/artifacts/allocations/empty.json Allocation fixture: empty set.
crates/metabench/tests/fixtures/artifacts/allocations/duplicate.json Allocation fixture: duplicate benchmark entries.
crates/metabench/src/mode.rs Mode enum + parsing/display and unit tests.
crates/metabench/src/lib.rs Public crate docs/exports + main! entry macro for benchmarks.
crates/metabench/src/group.rs Runtime benchmark registry, filtering, and wildcard matching + tests.
crates/metabench/src/fixture.rs Benchmark case/fixture traits + prepared output helper.
crates/metabench/src/error.rs Public error model and formatting + tests.
crates/metabench/src/engines.rs bitflags engine selection model and mapping to Mode.
crates/metabench/src/artifact.rs Artifact parsing/writing for Criterion/Gungraun/perf/allocations + tests.
crates/metabench/src/arguments.rs CLI argument parsing/routing + tests.
crates/metabench/README.md Generated crate README for metabench.
crates/metabench/CHANGELOG.md New changelog entry for crate introduction.
crates/metabench/Cargo.toml New metabench crate manifest, benches, deps, and metadata.
crates/metabench/build.rs Build script exporting target/profile metadata.
crates/metabench/benches/stateless_no_cases.rs Example bench target: stateless/no-cases registration.
crates/metabench/benches/stateless_cases.rs Example bench target: stateless/data-driven cases.
crates/metabench/benches/stateful_no_cases.rs Example bench target: stateful/no-cases via SimpleFixture.
crates/metabench/benches/stateful_cases/main.rs Example multi-file bench target entrypoint.
crates/metabench/benches/stateful_cases/hashmap.rs Example stateful data-driven bench group + cases.
crates/metabench_macros/tests/ui/support.rs UI-test support “mini runtime” surface for macro expansion.
crates/metabench_macros/tests/ui/pass/registration_selection.rs Pass UI test: group naming + selection behavior.
crates/metabench_macros/tests/ui/pass/fixtures.rs Pass UI test: fixtures/cases/stateful patterns.
crates/metabench_macros/tests/ui/fail/unsafe_method.stderr Fail UI test output: unsafe method rejection.
crates/metabench_macros/tests/ui/fail/unsafe_method.rs Fail UI test: unsafe method in benchmarks impl.
crates/metabench_macros/tests/ui/fail/unknown_argument.stderr Fail UI test output: unknown attribute argument.
crates/metabench_macros/tests/ui/fail/unknown_argument.rs Fail UI test: unsupported benchmarks attribute args.
crates/metabench_macros/tests/ui/fail/trait_impl.stderr Fail UI test output: trait impl rejected.
crates/metabench_macros/tests/ui/fail/trait_impl.rs Fail UI test: benchmarks applied to trait impl.
crates/metabench_macros/tests/ui/fail/non_path_group.stderr Fail UI test output: non-path self type rejected.
crates/metabench_macros/tests/ui/fail/non_path_group.rs Fail UI test: invalid group type.
crates/metabench_macros/tests/ui/fail/mutable_case.stderr Fail UI test output: mutable case ref rejected.
crates/metabench_macros/tests/ui/fail/mutable_case.rs Fail UI test: &mut Case parameter.
crates/metabench_macros/tests/ui/fail/multiple_arguments.stderr Fail UI test output: multiple params rejected.
crates/metabench_macros/tests/ui/fail/multiple_arguments.rs Fail UI test: multiple parameters.
crates/metabench_macros/tests/ui/fail/invalid_signature.stderr Fail UI test output: async method rejected.
crates/metabench_macros/tests/ui/fail/invalid_signature.rs Fail UI test: async method in benchmarks impl.
crates/metabench_macros/tests/ui/fail/invalid_receiver.stderr Fail UI test output: by-value receiver rejected.
crates/metabench_macros/tests/ui/fail/invalid_receiver.rs Fail UI test: self receiver.
crates/metabench_macros/tests/ui/fail/invalid_group_name.stderr Fail UI test output: group name validation.
crates/metabench_macros/tests/ui/fail/invalid_group_name.rs Fail UI test: group name contains /.
crates/metabench_macros/tests/ui/fail/invalid_benchmark_name.stderr Fail UI test output: benchmark name validation.
crates/metabench_macros/tests/ui/fail/invalid_benchmark_name.rs Fail UI test: benchmark name contains /.
crates/metabench_macros/tests/ui/fail/inline_method.stderr Fail UI test output: inline attribute rejected.
crates/metabench_macros/tests/ui/fail/inline_method.rs Fail UI test: method annotated #[inline].
crates/metabench_macros/tests/ui/fail/generic_method.stderr Fail UI test output: generic method rejected.
crates/metabench_macros/tests/ui/fail/generic_method.rs Fail UI test: generic method.
crates/metabench_macros/tests/ui/fail/generic_impl.stderr Fail UI test output: generic impl rejected.
crates/metabench_macros/tests/ui/fail/generic_impl.rs Fail UI test: generic impl block.
crates/metabench_macros/tests/ui/fail/empty_impl.stderr Fail UI test output: empty impl rejected.
crates/metabench_macros/tests/ui/fail/empty_impl.rs Fail UI test: empty impl.
crates/metabench_macros/tests/ui/fail/duplicate_names.stderr Fail UI test output: duplicate benchmark names.
crates/metabench_macros/tests/ui/fail/duplicate_names.rs Fail UI test: duplicate names via benchmark overrides.
crates/metabench_macros/tests/ui/fail/duplicate_benchmark_attribute.stderr Fail UI test output: duplicate benchmark attr.
crates/metabench_macros/tests/ui/fail/duplicate_benchmark_attribute.rs Fail UI test: duplicate #[benchmark] usage.
crates/metabench_macros/tests/ui/fail/duplicate_argument.stderr Fail UI test output: duplicate attribute argument.
crates/metabench_macros/tests/ui/fail/duplicate_argument.rs Fail UI test: repeated name = argument.
crates/metabench_macros/tests/ui/fail/const_method.stderr Fail UI test output: const method rejected.
crates/metabench_macros/tests/ui/fail/const_method.rs Fail UI test: const method.
crates/metabench_macros/tests/ui/fail/benchmarks_on_function.stderr Fail UI test output: attribute placement error.
crates/metabench_macros/tests/ui/fail/benchmarks_on_function.rs Fail UI test: benchmarks attribute on fn.
crates/metabench_macros/tests/ui/fail/benchmark_outside_impl.stderr Fail UI test output: benchmark attribute placement error.
crates/metabench_macros/tests/ui/fail/benchmark_outside_impl.rs Fail UI test: benchmark attribute outside benchmarks impl.
crates/metabench_macros/tests/ui/fail/associated_item.stderr Fail UI test output: associated items rejected.
crates/metabench_macros/tests/ui/fail/associated_item.rs Fail UI test: associated const in impl.
crates/metabench_macros/tests/ui.rs trybuild harness for downstream macro UI tests.
crates/metabench_macros/src/lib.rs Proc-macro entry points delegating to impl crate, with coverage exclusions.
crates/metabench_macros/README.md Generated README for metabench_macros.
crates/metabench_macros/CHANGELOG.md New changelog for metabench_macros.
crates/metabench_macros/Cargo.toml Proc-macro crate manifest + coverage-gate override.
crates/metabench_macros_impl/src/lib.rs Macro implementation: validates impls/methods and generates registrations.
crates/metabench_macros_impl/README.md Generated README for metabench_macros_impl.
crates/metabench_macros_impl/CHANGELOG.md New changelog for metabench_macros_impl.
crates/metabench_macros_impl/Cargo.toml Macro-impl crate manifest and deps (syn2/quote/etc).
Cargo.toml Workspace deps updated and new workspace members/deps added for metabench.
Cargo.lock Lockfile updated for new/updated dependencies.
.spelling Adds metabench-related vocabulary to spellcheck allowlist.
.github/workflows/main.yml Adds metabench registration step; excludes metabench from Miri.
Review details
  • Files reviewed: 122/126 changed files
  • Comments generated: 8
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/main.yml Outdated
Comment thread crates/metabench/src/arguments.rs
Comment thread crates/metabench/src/artifact.rs
Comment thread crates/metabench/src/error.rs
Comment thread crates/metabench/src/group.rs
Comment thread crates/metabench/src/lib.rs
Comment thread crates/metabench/src/mode.rs

Copilot AI left a comment

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.

🔵 Needs a closer look

It introduces a large new benchmarking subsystem (multiple crates + workspace/CI integration) that warrants focused human review of overall design, long-term maintenance costs, and operational behavior.

Review details

Suppressed comments (1)

crates/metabench/src/group.rs:357

  • When selecting without an identity manifest, an out-of-range cases.nth(index) is reported as Error::UnknownBenchmark(index.to_string()), which produces a confusing message (treating an index as a benchmark name). Consider including context that this is a case index.
    let (group, benchmark) = cases.nth(index).ok_or_else(|| Error::UnknownBenchmark(index.to_string()))?;
  • Files reviewed: 127/131 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/metabench/src/group.rs Outdated
Comment thread crates/metabench/tests/report_api.rs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

The new public-facing rustdoc examples in metabench use ? without a fn main() -> Result<...> wrapper, so the documentation snippets should be adjusted to be self-contained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/metabench/src/mode.rs Outdated
Comment thread crates/metabench/src/report.rs Outdated

Copilot AI left a comment

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.

🔵 Needs a closer look

It introduces multiple new crates, a new macro-based registration surface, and CI/dependency graph changes that warrant final human validation.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (a312e8b) to head (196e551).

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #732    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files         583      584     +1     
  Lines       62930    63327   +397     
========================================
+ Hits        62930    63327   +397     
Flag Coverage Δ
linux 100.0% <100.0%> (?)
linux-arm 100.0% <100.0%> (?)
windows 100.0% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

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.

🟡 Changes recommended

There are correctness/maintainability issues in new code (notably a JSON assertion comparing Value to an integer and insufficiently-informative expect() messages) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/metabench/benches/stateful_cases/hashmap.rs Outdated
Comment thread crates/metabench/tests/report_api.rs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

The report baseline logic can classify allocation regressions based on allocated_bytes shifts without exposing that shift in the public report model, which risks confusing downstream consumers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/report.rs

Copilot AI left a comment

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.

🟡 Changes recommended

The runner uses predictable temp-file/FIFO paths in a shared temp directory, which is vulnerable to interference on multi-user systems and should be hardened before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/runner.rs:705

  • create_worker_token uses a predictable path under the global temp directory. On multi-user systems this can be interfered with (pre-creating the path, symlink tricks, etc.), causing spurious failures or writing/deleting an unintended file. Prefer a securely-created unique temp file (O_EXCL + random name) from tempfile and pass that path to the worker.
fn create_worker_token() -> Result<std::path::PathBuf, Error> {
    let sequence = WORKER_SEQUENCE.fetch_add(1, Ordering::Relaxed);
    let token = env::temp_dir().join(format!("metabench-worker-{}-{sequence}.token", std::process::id()));
    fs::write(&token, []).map_err(Error::CreateWorkerToken)?;
    Ok(token)
}
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/runner.rs Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 19:19

Copilot AI left a comment

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.

🟡 Changes recommended

Artifact parsing/path generation has a couple of correctness/robustness gaps (perf pcnt-running validation and unbounded perf artifact filename length) that should be tightened before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread crates/metabench/src/artifact.rs Outdated
Comment thread crates/metabench/src/artifact.rs

Copilot AI left a comment

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.

🟡 Changes recommended

The worker filter env-var path accepts empty filters without validation, leading to inconsistent and potentially misleading error behavior compared to the CLI filter path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/group.rs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

The proc-macro implementation currently consumes any attribute ending in ::benchmark, which can conflict with unrelated attributes and should be narrowed to metabench’s intended forms.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench_macros_impl/src/lib.rs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Perf-mode measurements currently include black_box overhead because the Direct backend stops measurement after black-boxing the workload output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/bencher.rs:277

  • Same issue in the setup-based Direct backend: black_box(&output) happens before control.stop(), so perf counters (and any other MeasurementControl) include black-box overhead in the measured span. Consider stopping measurement immediately after the workload returns, then black-boxing and running cleanup outside the measured region (i.e., call control.stop() before black_box(&output) and cleanup(output)).
        Backend::Direct { iterations, control } => {
            for _ in 0..*iterations {
                let input = black_box(setup());
                if let Err(error) = control.start() {
                    bencher.failure = Some(error);
                    return;
                }
                let output = match catch_unwind(AssertUnwindSafe(|| invoke_with_input(&mut routine, input))) {
                    Ok(output) => output,
                    Err(payload) => {
                        let _ = catch_unwind(AssertUnwindSafe(|| control.stop()));
                        resume_unwind(payload);
                    }
                };
                black_box(&output);
                let stop = catch_unwind(AssertUnwindSafe(|| control.stop()));
                cleanup(output);
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/bencher.rs

Copilot AI left a comment

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.

🔵 Needs a closer look

It introduces a large new benchmarking framework (runtime + macros + CI/dependency updates), which warrants final human review for API/maintenance and ecosystem impact.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread Cargo.toml

Copilot AI left a comment

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.

🔵 Needs a closer look

It introduces multiple new crates (including proc macros) plus workspace/CI/dependency changes that warrant final human review for cross-workspace impact.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI left a comment

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.

🟡 Changes recommended

crates/metabench_macros_impl/src/lib.rs currently deviates from the repo’s established coverage configuration pattern for test modules, and dependency additions introduce a duplicated nix version that should be evaluated/aligned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench_macros_impl/src/lib.rs:315

  • Repository convention is to exclude unit-test modules from coverage measurement by placing #[cfg_attr(coverage_nightly, coverage(off))] immediately above #[cfg(test)] mod tests (for example crates/internity_macros_impl/src/lib.rs:56-58). Add the same attribute here so test-only lines don't count toward coverage reporting.
  • Files reviewed: 127/131 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread Cargo.toml
Comment thread crates/metabench_macros_impl/src/lib.rs

Copilot AI left a comment

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.

🟡 Changes recommended

A small but concrete maintainability issue remains in crates/metabench/src/arguments.rs where --allocations-arg is still implied as supported despite being intentionally removed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/arguments.rs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Allocation-mode measurement currently includes black_box(&output) inside the allocation span in Bencher, which can skew the tracked allocation window and should be reordered.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/bencher.rs:258

  • In allocation-tracking mode for setup-based benchmarks, black_box(&output) runs before the measurement span is dropped, so black-boxing is included in the measured allocation window. Drop the span immediately after the workload returns, then black-box / cleanup outside measurement to keep boundaries consistent across backends.
                let input = black_box(setup());
                let span = operation.measure_process().iterations(1);
                let output = invoke_with_input(&mut routine, input);
                black_box(&output);
                drop(span);
                cleanup(output);
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/src/bencher.rs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: b930a398-53c5-4301-a68d-1274a378b908

Copilot AI left a comment

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.

🟡 Changes recommended

The metabench crate’s public __private re-exports should be reflected in its cargo_check_external_types allowlist to keep metadata consistent with the exposed API surface.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 127/131 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread crates/metabench/Cargo.toml
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.

2 participants