-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Setup and use new benchmarking harness #8511
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
1c1a8f5 to
2173991
Compare
2173991 to
11f2870
Compare
|
Haven't checked if it's unrelated to your changes, but when trying to run |
Wumpf
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| //! 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"); |
There was a problem hiding this comment.
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" { |
There was a problem hiding this comment.
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" { |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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; | |||
|
|
|||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
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!
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.
Testing
Manual testing with CLI and it is part of the test suite.
Squash or Rebase?
Izza.