-
Notifications
You must be signed in to change notification settings - Fork 13.3k
test: Display benchmark results with thousands separators #26068
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
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
r? @huonw I think scientific notation is too hard to read. The nicest improvement is rounding to sig figs, units are a bit tricky to notice too. |
Maybe four sigfigs & 1 sigfig deviation is better example
Example from real benchmarks
|
Hm, as you say, the distinction between units is very subtle, especially the two most common unfortunately (n vs. u). |
More than anything it seems harder to "follow the trend" visually. Actual On Mon, Jun 8, 2015 at 3:37 PM, Huon Wilson notifications@github.com
|
Let's do thousands separators instead? |
That might be better yeah. |
They look like this
I think that's good enough. We could round it, but more correct was already harder to read. |
Updated branch. I guess the thousands formatting was pretty quick and dirty. |
let base = 10_usize.pow(pow); | ||
if pow == 0 || n / base != 0 { | ||
if first { | ||
output.write_fmt(format_args!("{}", n / base)).ok(); |
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.
ok
-> unwrap()
?
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.
I guess that's more idiomatic. On a string it doesn't matter.
Example display: ``` running 9 tests test a ... bench: 0 ns/iter (+/- 0) test b ... bench: 52 ns/iter (+/- 0) test c ... bench: 88 ns/iter (+/- 0) test d ... bench: 618 ns/iter (+/- 111) test e ... bench: 5,933 ns/iter (+/- 87) test f ... bench: 59,280 ns/iter (+/- 1,052) test g ... bench: 588,672 ns/iter (+/- 3,381) test h ... bench: 5,894,227 ns/iter (+/- 303,489) test i ... bench: 59,112,382 ns/iter (+/- 1,500,110) ``` Fixes rust-lang#10953 Fixes rust-lang#26109
Updated to use .unwrap(). |
@bors r+ (Now there's just the question of localising it properly...) |
📌 Commit 2b50c15 has been approved by |
⌛ Testing commit 2b50c15 with merge f1313da... |
💔 Test failed - auto-mac-64-opt |
@bors: retry On Mon, Jun 8, 2015 at 9:28 PM, bors notifications@github.com wrote:
|
⌛ Testing commit 2b50c15 with merge a9f50bd... |
test: Display benchmark results with thousands separators Example display: ``` running 9 tests test a ... bench: 0 ns/iter (+/- 0) test b ... bench: 52 ns/iter (+/- 0) test c ... bench: 88 ns/iter (+/- 0) test d ... bench: 618 ns/iter (+/- 111) test e ... bench: 5,933 ns/iter (+/- 87) test f ... bench: 59,280 ns/iter (+/- 1,052) test g ... bench: 588,672 ns/iter (+/- 3,381) test h ... bench: 5,894,227 ns/iter (+/- 303,489) test i ... bench: 59,112,382 ns/iter (+/- 1,500,110) ``` Fixes #10953 Fixes #26109
test: Display benchmark results with thousands separators
Example display:
Fixes #10953
Fixes #26109