Skip to content
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.

Sign up for GitHub

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

Jump to bottom

"bencher" output format doesn't quite match what libtest creates #704

Open
d-e-s-o opened this issue Jul 21, 2023 · 1 comment
Open

"bencher" output format doesn't quite match what libtest creates #704

d-e-s-o opened this issue Jul 21, 2023 · 1 comment

Comments

@d-e-s-o
Copy link
Contributor

d-e-s-o commented Jul 21, 2023 •
edited
Loading

Thanks for adding the bencher output format. I have both libtest based benchmarks as well as criterion based ones, and I use this output format to have better comparability.

However, I noticed that the formats don't quite match up (which I will say kind of runs against the stated purpose: "it may be useful to support external tools that can parse this output"). Specifically, this is output generated by libtest benchmarks:

running 4 tests
test dwarf::units::tests::bench_function_parsing           ... bench:  13,660,119 ns/iter (+/- 70,841)
test dwarf::units::tests::bench_function_parsing_addr2line ... bench:  11,567,597 ns/iter (+/- 78,162)
test dwarf::units::tests::bench_line_parsing               ... bench:   4,888,411 ns/iter (+/- 957,070)
test dwarf::units::tests::bench_line_parsing_addr2line     ... bench:   5,278,859 ns/iter (+/- 336,390)

Compare that to criterion based ones:

test main/inspect :: lookup_dwarf ... bench:   208037937 ns/iter (+/- 3266769)
test main/normalize :: normalize_process ... bench:      182202 ns/iter (+/- 4353)
test main/symbolize :: symbolize_process ... bench:     4074900 ns/iter (+/- 122307)
test main/symbolize :: symbolize_dwarf ... bench:   140352319 ns/iter (+/- 3505828)
test main/symbolize :: symbolize_gsym ... bench:       42150 ns/iter (+/- 1559)

(some of those criterion benchmarks are in different groups; I haven't checked whether that has any impact)

There are two main differences that I can make out:

  • alignment of numbers is missing in criterion based output and
  • thousands separators are not present

Would it make sense and be possible to align the two more closely?
d-e-s-o added a commit to d-e-s-o/criterion.rs that referenced this issue Jul 21, 2023
@d-e-s-o
Include thousands-separators in bencher output
da915a2 
As mentioned in bheisler#704, the output generated when using the `bencher`
output format does not quite match up with that of libtest. One reason
being that thousands-separators are not emitted.
With this change we adjust the formatting logic for integers (which is
only used in `bencher` style reports) to emit thousands-separators, as
libtest does.

Signed-off-by: Daniel Müller <deso@posteo.net>
@d-e-s-o d-e-s-o mentioned this issue Jul 21, 2023
Merged
d-e-s-o added a commit to d-e-s-o/criterion.rs that referenced this issue Jul 21, 2023
@d-e-s-o
Include thousands-separators in bencher output
c9e80b5 
As mentioned in bheisler#704, the output generated when using the `bencher`
output format does not quite match up with that of libtest. One reason
being that thousands-separators are not emitted.
With this change we adjust the formatting logic for integers (which is
only used in `bencher` style reports) to emit thousands-separators, as
libtest does.

Signed-off-by: Daniel Müller <deso@posteo.net>
d-e-s-o added a commit to d-e-s-o/criterion.rs that referenced this issue Jul 21, 2023
@d-e-s-o
Include thousands-separators in bencher output
1114241 
As mentioned in bheisler#704, the output generated when using the `bencher`
output format does not quite match up with that of libtest. One reason
being that thousands-separators are not emitted.
With this change we adjust the formatting logic for integers (which is
only used in `bencher` style reports) to emit thousands-separators, as
libtest does.

Signed-off-by: Daniel Müller <deso@posteo.net>
@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented Jul 21, 2023 •
edited
Loading

Thousands separators don't seem too hard to add. See #705. As for the padding/alignment, here is the relevant logic in libtest, for reference:

https://github.com/rust-lang/rust/blob/557359f92512ca88b62a602ebda291f17a953002/library/test/src/console.rs#L299-L303

https://github.com/rust-lang/rust/blob/557359f92512ca88b62a602ebda291f17a953002/library/test/src/formatters/pretty.rs#L171-L181

I believe given the current state of the reporter logic that this requires a bit of a restructuring of internals to be feasible. Right now the reporter does not know of test names ahead of time from what I gather. If you have any suggestions, please let me know.

d-e-s-o added a commit to d-e-s-o/blazesym that referenced this issue Jul 21, 2023
@d-e-s-o
Properly align benchmark output
ad211dc 
Unfortunately criterion with the bencher output format does not align
benchmark results properly ([[0]]). As a result it is quite hard to read
the summary from a glimpse.
To work around this problem, this change "manually" adjusts the names
of our benchmark functions so that the corresponding results in the
summary report are aligned properly.

[[0]]: bheisler/criterion.rs#704

Signed-off-by: Daniel Müller <deso@posteo.net>
d-e-s-o added a commit to d-e-s-o/blazesym that referenced this issue Jul 21, 2023
@d-e-s-o
Properly align benchmark output
452c0e1 
Unfortunately criterion with the bencher output format does not align
benchmark results properly ([0]). As a result it is quite hard to read
the summary from a glimpse.
To work around this problem, this change "manually" adjusts the names
of our benchmark functions so that the corresponding results in the
summary report are aligned properly.

[0]: bheisler/criterion.rs#704

Signed-off-by: Daniel Müller <deso@posteo.net>
d-e-s-o added a commit to d-e-s-o/blazesym that referenced this issue Jul 21, 2023
@d-e-s-o
Properly align benchmark output
f3430c9 
Unfortunately criterion with the bencher output format does not align
benchmark results properly ([0]). As a result it is quite hard to read
the summary from a glimpse.
To work around this problem, this change "manually" adjusts the names
of our benchmark functions so that the corresponding results in the
summary report are aligned properly.

[0]: bheisler/criterion.rs#704

Signed-off-by: Daniel Müller <deso@posteo.net>
@d-e-s-o d-e-s-o mentioned this issue Jul 21, 2023
Merged
d-e-s-o added a commit to libbpf/blazesym that referenced this issue Jul 21, 2023
@d-e-s-o
Properly align benchmark output
cd6456c 
Unfortunately criterion with the bencher output format does not align
benchmark results properly ([0]). As a result it is quite hard to read
the summary from a glimpse.
To work around this problem, this change "manually" adjusts the names
of our benchmark functions so that the corresponding results in the
summary report are aligned properly.

[0]: bheisler/criterion.rs#704

Signed-off-by: Daniel Müller <deso@posteo.net>
lemmih pushed a commit that referenced this issue Aug 10, 2023
@d-e-s-o
Include thousands-separators in bencher output (#705)
daa84ca 
As mentioned in #704, the output generated when using the `bencher`
output format does not quite match up with that of libtest. One reason
being that thousands-separators are not emitted.
With this change we adjust the formatting logic for integers (which is
only used in `bencher` style reports) to emit thousands-separators, as
libtest does.

Signed-off-by: Daniel Müller <deso@posteo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@d-e-s-o