Skip to content

Conversation

@cwfitzgerald
Copy link
Member

@cwfitzgerald cwfitzgerald commented Nov 11, 2025

Connections

Closes #8508?

Description

This completely redoes our benchmarking to use a custom interface. This provides a number of benefits enumerated in the lib.rs file. Importantly it improves output to be easier to understand, makes the test code much simpler, and allows us to do test selection more intelligently.

image

Testing

Manual testing with CLI and it is part of the test suite.

Squash or Rebase?

Izza.

@Wumpf
Copy link
Member

Wumpf commented Nov 16, 2025

Haven't checked if it's unrelated to your changes, but when trying to run cargo bench --bench wgpu-benchmark I get a panic for unknown instruction 🤔

Running benchmark: naga::front

thread 'main' panicked at benches\benches\wgpu-benchmark\shader.rs:254:32:
called `Result::unwrap()` on an `Err` value: UnknownInstruction(2573)
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library\std\src\panicking.rs:697
   1: core::panicking::panic_fmt
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library\core\src\panicking.rs:75
   2: core::result::unwrap_failed
             at /rustc/6b00bc3880198600130e1cf62b8f8a93494488cc/library\core\src\result.rs:1732
   3: wgpu_benchmark::shader::frontends::closure$4
             at .\benches\wgpu-benchmark\shader.rs:254
   4: wgpu_benchmark::iter::iter_auto::closure$0
             at .\src\iter.rs:51
   5: wgpu_benchmark::iter::iter
             at .\src\iter.rs:24
   6: wgpu_benchmark::iter::iter_auto<wgpu_benchmark::shader::frontends::closure_env$4>
             at .\src\iter.rs:44
   7: wgpu_benchmark::shader::frontends
             at .\benches\wgpu-benchmark\shader.rs:234
   8: wgpu_benchmark::main
             at .\src\lib.rs:231
   9: wgpu_benchmark::main
             at .\benches\wgpu-benchmark\main.rs:101
  10: core::ops::function::FnOnce::call_once
             at C:\Users\Andreas\.rustup\toolchains\1.88-x86_64-pc-windows-msvc\lib\rustlib\src\rust\library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor stylistic notes but lgtm, makes sense and the output looks really nice!
Would be good though to figure out what the crash is about, I can poke a bit more later


//! Benchmarking framework for `wgpu`.
//!
//! This crate a basic framework for benchmarking. Its design is guided
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//! This crate a basic framework for benchmarking. Its design is guided
//! This crate is a basic framework for benchmarking. Its design is guided

}

let exact = args.contains("--exact");
let _no_capture = args.contains("--no-capture");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this for?

assert_eq!(fmt, "terse", "Only 'terse' format is supported.");
}
if let Some(ref baseline) = baseline_name {
if baseline == "previous" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: there's a constant for that in context.rs

}
}
if let Some(ref write_baseline) = write_baseline {
if write_baseline == "previous" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

///
/// Positive changes are red (regression), negative changes are green (improvement).
/// This represents changes for time durations. For throughput changes, the sign should be inverted.
fn get_change_color(change: f64) -> ColorSpec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are changes in percent? why not documented or named like that

@@ -0,0 +1,95 @@
use std::time::Duration;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authoring a benchmark I'd expect those to be on those on BenchmarkContext's impl.
Also, some doc string on when to use what would be good for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while we're on the topic if benchmark structure: I find it also a lil bit odd that every benchmark has to build up its own Vec of results and not have that being managed by the benchmark context. Not a big deal ofc, but makes this feel a bit raw 🤷

LoopControl::Time(Duration::from_secs(1))
};

while !control.finished(iterations, *durations.first().unwrap_or(&Duration::ZERO)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the duration picking behavior is a bit strange and needs documentation!

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.

wgpu benchmark does not work very well on Mac

2 participants