feat: switch continuous benchmarks to gungraun#222
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15cda8a777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match scenario.as_str() { | ||
| "deps" => run_resolve_dependencies(false).await, | ||
| "many_extensions" => run_resolve_many_extensions().await, | ||
| "pnp" => run_resolve_dependencies(true).await, |
There was a problem hiding this comment.
For the pnp scenario this now calls run_resolve_dependencies(true), which uses benchmark_requests() and therefore resolves the normal benches/package.json dependencies from the benches directory. The previous PnP benchmark resolved preact from fixtures/pnp/{1..10}, so this benchmark no longer exercises the Plug'n'Play fixture and can report healthy PnP numbers even if PnP-specific resolution regresses.
Useful? React with 👍 / 👎.
| for request in requests.iter().take(200) { | ||
| let _ = resolver.resolve(&context, request).await; |
There was a problem hiding this comment.
The many_extensions scenario now resolves each bare package request, so most work is normal package resolution and may finish via package metadata before probing the long extension list. The old benchmark deliberately resolved format!("{}/bad", request) to force failed extension attempts across the configured extensions, so this change makes the reported resolve_many_extensions benchmark measure a different, much cheaper path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s continuous benchmarking from CodSpeed to Gungraun by introducing a Gungraun-driven benchmark target and a dedicated benchmark driver binary, and updating CI to run the new benchmark and upload its artifacts.
Changes:
- Add a
resolver_bench_driverbinary to run benchmark scenarios used by Gungraun binary benchmarks. - Add a new
resolver_gungraunbenchmark target and wire it into CI (install valgrind + gungraun-runner; uploadtarget/gungraun). - Add
gungraundev-dependency and update lockfile accordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bin/resolver_bench_driver.rs | New benchmark driver binary that runs benchmark scenarios (deps/many_extensions/pnp). |
| benches/resolver_gungraun.rs | New Gungraun binary benchmark definition that invokes the driver binary per scenario. |
| Cargo.toml | Register new bench target and add gungraun dev-dependency. |
| Cargo.lock | Lockfile updates for gungraun and transitive deps. |
| .github/workflows/benchmark.yml | Switch workflow execution from CodSpeed to running cargo bench --bench resolver_gungraun and uploading Gungraun artifacts. |
Comments suppressed due to low confidence (1)
src/bin/resolver_bench_driver.rs:112
- The resolve result is discarded here as well, so this scenario can silently benchmark failed resolutions if the benchmark fixtures aren’t present. Consider validating the returned result and failing fast with an actionable message when resolution fails.
async fn run_resolve_many_extensions() {
let (context, requests) = benchmark_requests();
let resolver = resolver_with_many_extensions();
for request in requests.iter().take(200) {
let _ = resolver.resolve(&context, request).await;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ".js".to_string(), | ||
| ], | ||
| imports_fields: vec![], | ||
| exports_fields: vec![], |
| "pnp" => run_resolve_dependencies(true).await, | ||
| _ => { | ||
| eprintln!("unknown scenario: {scenario}. expected one of: deps, many_extensions, pnp"); | ||
| process::exit(2); | ||
| } |
| async fn run_resolve_dependencies(enable_pnp: bool) { | ||
| let (context, requests) = benchmark_requests(); | ||
| let resolver = rspack_resolver(enable_pnp); | ||
|
|
||
| for request in requests { | ||
| let _ = resolver.resolve(&context, &request).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78151f6f41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "pnp" => run_resolve_dependencies(true).await, | ||
| _ => { | ||
| eprintln!("unknown scenario: {scenario}. expected one of: deps, many_extensions, pnp"); | ||
| process::exit(2); |
There was a problem hiding this comment.
process::exit in the new default binary
This new src/bin target is included when the CI lint job runs cargo clippy --all-features -- -D warnings in .github/workflows/ci.yml, and the repository explicitly configures clippy::exit as a warning in Cargo.toml; with -D warnings, this process::exit call makes the lint job fail before the benchmark change can merge. Returning an error from main or using a panic/message path avoids tripping that lint.
Useful? React with 👍 / 👎.
| ], | ||
| imports_fields: vec![], | ||
| exports_fields: vec![], | ||
| enable_pnp: false, |
There was a problem hiding this comment.
enable_pnp for no-default-feature builds
When consumers or maintainers run this crate with default features disabled, ResolveOptions::enable_pnp is not present because it is behind #[cfg(feature = "yarn_pnp")]; moving this initializer into a default src/bin target means cargo check --no-default-features now fails even though the crate exposes yarn_pnp as an optional feature. The similar initializer above is already cfg-gated, so this one needs the same treatment.
Useful? React with 👍 / 👎.
| for request in requests { | ||
| let _ = resolver.resolve(&context, &request).await; |
There was a problem hiding this comment.
For the normal deps scenario, every resolver error is now discarded, so a regression that makes all benchmark dependencies fail to resolve still exits 0 and produces apparently valid Gungraun results; the previous benchmark had a preflight check that panicked on any unresolved dependency and also caught missing benches/node_modules setup. This should assert or propagate errors for the dependency scenario, while leaving intentionally failing scenarios separate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 598918bfb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| git worktree add /tmp/gungraun-pr-base "${BASE_SHA}" | ||
| pushd /tmp/gungraun-pr-base | ||
| pnpm install --dir benches --ignore-workspace | ||
| cargo bench --bench resolver_gungraun -- \ |
There was a problem hiding this comment.
On pull requests this step checks out github.event.pull_request.base.sha into /tmp/gungraun-pr-base and then runs cargo bench --bench resolver_gungraun there; cargo bench --help confirms --bench runs only the specified bench target. Because resolver_gungraun and its [[bench]] entry are introduced by this commit, the base worktree for this migration PR has no such bench target, so the new benchmark workflow fails before producing any comparison.
Useful? React with 👍 / 👎.
| #[bench::resolve_dependencies("deps")] | ||
| #[bench::resolve_many_extensions("many_extensions")] | ||
| #[bench::resolve_dependencies_pnp("pnp")] |
There was a problem hiding this comment.
The new workflow now runs only cargo bench --bench resolver_gungraun, but this Gungraun bench registers only deps, many_extensions, and pnp. The previous continuous benchmark also covered multi-thread resolution plus the two symlink-resolution cases in benches/resolver.rs, so regressions in concurrent cache behavior or symlink handling will no longer be measured by CI after this switch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce32bb95a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,127 @@ | |||
| use std::{env, fs::read_to_string, path::PathBuf, process}; | |||
There was a problem hiding this comment.
When the Gungraun workflow runs this new binary target, the code under Callgrind is resolver_bench_driver, but this file never installs the NeverGrowInPlaceAllocator<mimalloc::MiMalloc> that the existing benches/resolver.rs benchmark still uses. On Linux CI this means the migrated benchmark now measures the system allocator path instead of the allocator setup the benchmark was explicitly using before, so allocation-heavy resolver changes can look better or worse for reasons unrelated to resolver performance; the allocator wrapper needs to live in the benchmarked driver process, not just in the old Criterion bench.
Useful? React with 👍 / 👎.
Summary
Validation