feat: introduce metabench, a unified benchmarking framework - #732
feat: introduce metabench, a unified benchmarking framework#732Martin Taillefer (geeknoid) wants to merge 1 commit into
Conversation
|
| 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 | — | |
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.
There was a problem hiding this comment.
🟡 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
metabenchruntime crate plusmetabench_macros/metabench_macros_implproc-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.
d94c06f to
f6da4cc
Compare
There was a problem hiding this comment.
🔵 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 asError::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
f6da4cc to
fdaa7be
Compare
There was a problem hiding this comment.
🟡 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
fdaa7be to
95a31f3
Compare
There was a problem hiding this comment.
🔵 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 Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
95a31f3 to
bd22cf4
Compare
There was a problem hiding this comment.
🟡 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
bd22cf4 to
372834d
Compare
There was a problem hiding this comment.
🟡 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
372834d to
042edcd
Compare
There was a problem hiding this comment.
🟡 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
tempfileand 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
There was a problem hiding this comment.
🟡 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
87d48b6 to
4da87e5
Compare
There was a problem hiding this comment.
🟡 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
4da87e5 to
b7a5d6f
Compare
b7a5d6f to
7a56330
Compare
There was a problem hiding this comment.
🟡 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
7a56330 to
7366e2a
Compare
7366e2a to
2775e31
Compare
There was a problem hiding this comment.
🟡 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
Directbackend:black_box(&output)happens beforecontrol.stop(), so perf counters (and any otherMeasurementControl) 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., callcontrol.stop()beforeblack_box(&output)andcleanup(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
2775e31 to
f6f0abe
Compare
There was a problem hiding this comment.
🔵 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
f6f0abe to
3473439
Compare
3473439 to
73e66aa
Compare
There was a problem hiding this comment.
🔵 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
There was a problem hiding this comment.
🟡 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 examplecrates/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
There was a problem hiding this comment.
🟡 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
There was a problem hiding this comment.
🟡 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
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b930a398-53c5-4301-a68d-1274a378b908
There was a problem hiding this comment.
🟡 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
No description provided.