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

Compliant time units in CLI report and results export #71

Closed
kirillbobyrev opened this issue Aug 16, 2018 · 9 comments
Closed

Compliant time units in CLI report and results export #71

kirillbobyrev opened this issue Aug 16, 2018 · 9 comments
Labels
feature-request good first issue Good for newcomers

Comments

@kirillbobyrev
Copy link

When passing --export-markdown the resulted Markdown file includes information about Mean and Min..Max measured in ms even when the original result report printed to stdout is presented in seconds. I think it might be better if the produced report would use the same units since it's hard to read very long ms numbers.

Instructions for the behavior reproduction:

hyperfine 'sleep 5' --export-markdown results.md

Command line output:

kbobyrev@kbobyrev ~/d/m/profile> hyperfine 'sleep 5' --export-markdown results.md
Benchmark #1: sleep 5
  Time (mean ± σ):      5.002 s ±  0.000 s    [User: 1.1 ms, System: 2.0 ms]
  Range (min … max):    5.002 s …  5.003 s

results.md:

Command Mean [ms] Min…Max [ms]
sleep 5 5002.3 ± 0.2 5002.1…5002.7
@kirillbobyrev kirillbobyrev changed the title [Feature request] Time units in results export [Feature request] Compliant time units in CLI report and results export Aug 16, 2018
@sharkdp
Copy link
Owner

sharkdp commented Aug 18, 2018

Yes - we should definitely support this. Thank you for the feedback.

@jasonpeacock
Copy link
Contributor

I'll give this a try...

It looks like BenchmarkResult stores all values internally as seconds, and the other formats (csv, json) don't perform any conversion - they always output as seconds.

This change should be just removing the MULTIPLIER from the markdown exporter, then it will have the same always-output-seconds behavior as the other format exporters. This will change the markdown results for all users from milliseconds to seconds, a UX breaking change.

But this doesn't fix the actual root cause of the issue - the results export may still differ in units from the CLI report because the CLI report will automatically adjust format between seconds and milliseconds if it's <1s while the results export is always seconds for all metrics.

I'll submit a feature request to add an option to choose (force) the units for both the CLI & results, then they will always match, if desired.

@jasonpeacock jasonpeacock mentioned this issue Sep 9, 2018
Closed
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 9, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
08aeb10 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

* Change the markdown results exporter to also use the default seconds
unit of `BenchmarkResult`.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

* Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
@sharkdp
Copy link
Owner

sharkdp commented Sep 9, 2018 •
edited
Loading

It looks like BenchmarkResult stores all values internally as seconds, and the other formats (csv, json) don't perform any conversion - they always output as seconds.

CSV and JSON are formats to be automatically processed (where a hard-coded unit is the right choice, in my opinon), but Markdown is for humans.

This change should be just removing the MULTIPLIER from the markdown exporter, then it will have the same always-output-seconds behavior as the other format exporters. This will change the markdown results for all users from milliseconds to seconds, a UX breaking change.

I would prefer if we show the same unit that is also shown in the terminal (which is automatically inferred).

@sharkdp sharkdp changed the title [Feature request] Compliant time units in CLI report and results export Compliant time units in CLI report and results export Sep 9, 2018
@jasonpeacock
Copy link
Contributor

I would prefer if we show the same unit that is also shown in the terminal (which is automatically infered).

Sure, can do. I'll update the PR with this support.

@jasonpeacock
Copy link
Contributor

I've updated the PR, but it's failing the some of the build checks b/c the older 1.24 version of Rust doesn't like my modern match statements :)

I'm not sure how old of Rust you're supporting, let me know if you want me to make the changes to support 1.24 in addition to stable?

@jasonpeacock
Copy link
Contributor

Also, do you want the changes squashed?

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
a231c79 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
@jasonpeacock
Copy link
Contributor

I've gone ahead and squashed the commits, and made the match statements satisfy Rust 1.24.0.

jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
e912e0f 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Sep 15, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
596e689 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
jasonpeacock added a commit to jasonpeacock/hyperfine that referenced this issue Oct 28, 2018
@jasonpeacock
Change markdown export to use seconds, closes sharkdp#71
0e9f6af 
The markdown results exporter was using milliseconds, but the other
results exporters are using the default seconds unit.

Apply some refactoring so the same logic used to select units for the
CLI output can be used to select the units for the markdown exporter.

Now the units of the markdown exporter match that of the CLI.

When there are multiple benchmarks, the CLI will choose the
appropriate unit for each benchmark separately. In this case, the
markdown exporter will use the first benchmark to select the units for
all the results.

*This will change the markdown results for all users from milliseconds
to seconds, a UX breaking change.*

Add unit tests for the markdown exporter to verify the output.

Issue sharkdp#80 proposes a new option to choose the units used for both the
CLI report and results export.
@sharkdp
Copy link
Owner

sharkdp commented Oct 28, 2018

Fixed by @jasonpeacock in #84

@sharkdp
Copy link
Owner

sharkdp commented Nov 21, 2018

Released in v1.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kirillbobyrev @sharkdp @jasonpeacock