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.

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

Added option to manually specify a reference to compare the results to. #744

Merged
merged 4 commits into from
Jun 30, 2024

Conversation

JanEricNitschke
Copy link
Contributor

I was interested in the functionality discussed in #579 and #577.

Sadly the former PR has been stale for over a year.

So i have tried to effectively take the work from there, make it fit with the changes that have happend since then and also address the comments in the PR.

There are currently still two points that i am myself unsure about.

I have used the standard std::cmp::Ordering for the ordering compared to the reference. However, i feel that the naming is not actually clear at a first glance. Namely that Ordering::Less corresponds to a faster time (the runtime is less). It might be worth to create a distinct enum that maps to this (converts from) as suggested in the original PR review.

I am also not sure if the statements faster than and slower than are the best. Particularly the latter feels a bit unintuitive to me.
Maybe something like 0.2 times as fast would be better than 5.0 times slower?

@sharkdp
Copy link
Owner

sharkdp commented Jun 23, 2024

Thank you very much for your contribution. Sorry for the delayed review.

This looks very good from a first glance. How does this new --reference option interplay with parametrized benchmarks?

Also, please consider creating (integration) tests for this new option.

@JanEricNitschke
Copy link
Contributor Author

Thanks for having a look.

The reference is specified separately from all other options, so also separately from a scan and has to be specified fully.

$ hyperfine --reference "sleep 0.4" --parameter-scan delay 0.3 0.7 -D 0.2 'sleep {delay}'
Benchmark 1: sleep 0.4
  Time (mean ± σ):     430.1 ms ±   1.3 ms    [User: 0.0 ms, System: 19.1 ms]
  Range (min … max):   426.9 ms … 431.3 ms    10 runs

Benchmark 2: sleep 0.3
  Time (mean ± σ):     321.3 ms ±   1.0 ms    [User: 2.9 ms, System: 9.0 ms]
  Range (min … max):   319.8 ms … 322.9 ms    10 runs

Benchmark 3: sleep 0.5
  Time (mean ± σ):     523.5 ms ±   1.2 ms    [User: 2.9 ms, System: 10.5 ms]
  Range (min … max):   521.6 ms … 525.1 ms    10 runs

Benchmark 4: sleep 0.7
  Time (mean ± σ):     726.4 ms ±   1.7 ms    [User: 0.0 ms, System: 13.1 ms]
  Range (min … max):   722.8 ms … 728.3 ms    10 runs

Summary
  sleep 0.4 ran
    1.34 ± 0.01 times slower than sleep 0.3
    1.22 ± 0.00 times faster than sleep 0.5
    1.69 ± 0.01 times faster than sleep 0.7

Will have a look at addings tests. Thanks again!

@JanEricNitschke
Copy link
Contributor Author

Currently working on the tests. Was thinking about how --prepare, --conclude and --command-name should work.

Currently i have modified it so that prepare and conclude affect the reference and the number needed is (n_commands + 1/0).

For command name the easiest would be to not affect the reference, but instead add a separate option --reference-name.

println!(
"{}{} times faster than {}",
"{}{} times {} than {}",
Copy link
Owner

@sharkdp sharkdp Jun 30, 2024

Choose a reason for hiding this comment

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

Can we maybe fix this for the Equal case? Even if it is unlikely to happen in practice (?)

  • "2 times faster than …" makes sense
  • "2 times slower than …" makes sense
  • "2 times as fast than …" is wrong and makes no sense.

Maybe we could print: "As fast (1.0 ± 0.2) as …"

Would probably make sense to add a test for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it as you suggested. Although i am not 100% on the alignment of the message. At the moment i have prefixed it with 4 spaces so that it is aligned with single digit double decimal ratios (2.34). The ratio itself for the same time ones will always be (1.00)

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much for the updates and explanations. This looks great. Only have two minor comments.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit 13547e9 into sharkdp:master Jun 30, 2024
15 checks passed
@tony
Copy link

tony commented Sep 11, 2024

For command name the easiest would be to not affect the reference, but instead add a separate option --reference-name.

@sharkdp @JanEricNitschke Did --reference end up supporting the command being named? Should I / one of us file a separate issue to remind a follow up is needed on this?

@JanEricNitschke
Copy link
Contributor Author

@tony Looking back over the code it looks like this was not added. So i think you can file an issue. Although i feel that this would also be a good option for a PR.

I think that shouldnt be too difficult and i also created this here as my first (and only) on the project because i wanted it.

@miluoshi
Copy link

Thank you for adding this feature. A follow-up question in general: Shouldn't be also the README.md always updated when adding a new feature, so it can be discovered not only from hyperfine --help?

@JanEricNitschke
Copy link
Contributor Author

@miluoshi Probably a good idea. I think you can probably create a PR to add documentation for this. Otherwise i might do it when i get around to it

niklasdewally added a commit to niklasdewally/hyperfine that referenced this pull request Mar 15, 2025
Add --reference-name option to give a reference command a name.

This was discussed in the PR that added --reference, sharkdp#744, but
was unfortunately never implemented.
niklasdewally added a commit to niklasdewally/hyperfine that referenced this pull request Mar 15, 2025
Add --reference-name option to give a reference command a name.

This was discussed in the PR that added --reference, sharkdp#744, but
was unfortunately never implemented.
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.

None yet

4 participants