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

PR for relative speed as part of exported data? #127

Closed
mathiasrw opened this issue Jan 3, 2019 · 13 comments · Fixed by #175
Closed

PR for relative speed as part of exported data? #127

mathiasrw opened this issue Jan 3, 2019 · 13 comments · Fixed by #175

Comments

@mathiasrw
Copy link

mathiasrw commented Jan 3, 2019

The CLI output has a really great summary formatted like:

Summary
  'curl google.com' ran
    2.93 ± 2.29 times faster than 'wget google.com'

The relative speed provided (2.93) is, in my opinion, a major take-a-way when reading speed tests. Unfortunately, this information is not (directly) part of the exported data for markdown, json and csv.

Would you be willing to accept a PR adding information about the relative speed to the exported data?

@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2019

Seems like a great idea, thank you for the feedback. I'd be glad to take a PR.

@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2019

.. but maybe we should export it as "relative time" instead where 1.0 would be the value for the fastest? I think this could be easier to read next to the "Time [ms]" column. What do you think?

@mathiasrw
Copy link
Author

mathiasrw commented Jan 3, 2019

Thank you for being so open about inputs :)

For the csv and json, I agree that "Relative time" is the most accurate. I also believe the data should contain the existing two decimal number as displayed in the existing summary. However, with the markdown, the visuals get a bit off as the title is long but the content short.

I have been playing with the options and the version I find the most appealing is

Speed Command Mean [s] Min [s] Max [s]
2.6x sed -i '' 's/<.*?>//g' testfile 0.672 ± 0.013 0.649 0.689
1.6x rexreplace '<.*?>' '' testfile 0.417 ± 0.019 0.394 0.447
1.0x perl -pi -e 's/<.*?>//g' testfile 0.264 ± 0.017 0.246 0.292
3.1x awk '{gsub(/<.*?>/,"")}' testfile 0.828 ± 0.019 0.806 0.870

I know - I have been a bit cheeky to place it as the first column and to rename it to "Speed" while cutting down to only one decimal, adding an x and even changed min-max into to two different columns (as in the original issue #44) but it really gives the calmest expression in my opinion. I like it as I experience a low cognitive barea to quickly identify fastest/ slowest command when the relative speed/time is so close to the beginning of the command.

What do you think?

To compare, here is an example of placing it next to the Mean column and keeping data as close to the existing implementation as possible

Command Mean [s] Relative time Min…Max [s]
sed -i '' 's/<.*?>//g' testfile 0.672 ± 0.013 2.55 0.649…0.689
rexreplace '<.*?>' '' testfile 0.417 ± 0.019 1.58 0.394…0.447
perl -pi -e 's/<.*?>//g' testfile 0.264 ± 0.017 1.00 0.246…0.292
awk '{gsub(/<.*?>/,"")}' testfile 0.828 ± 0.019 3.14 0.806…0.870

@sharkdp
Copy link
Owner

sharkdp commented Jan 3, 2019

First off, independent from this ticket: splitting "Min…Max" into two columns sounds like a good idea!

Concerning your examples: The first table looks better, indeed. However, "speed" sounds like I should be looking for the highest value (fastest speed), but awk ("3.1 x") is the slowest. If we want to use the "Speed" column title, I would probably vote for reciprocal values (with a speed of 1.0 for the slowest).

As for the lower table. What if the column would be on the very right and would just be labeled "Relative"?

@mathiasrw
Copy link
Author

So something like this?

Command Mean [s] Min [s] Max [s] Relative
sed -i '' 's/<.*?>//g' testfile 0.672 ± 0.013 0.649 0.689 2.6x
rexreplace '<.*?>' '' testfile 0.417 ± 0.019 0.394 0.447 1.6x
perl -pi -e 's/<.*?>//g' testfile 0.264 ± 0.017 0.246 0.292 1.0x
awk '{gsub(/<.*?>/,"")}' testfile 0.828 ± 0.019 0.806 0.870 3.1x

@sharkdp
Copy link
Owner

sharkdp commented Jan 7, 2019

Looks good to me, what do you think?

I'd maybe remove the x from 2.6x..

@mathiasrw
Copy link
Author

Ok - got a version working where markdown spits out the right data 🎉

Command Mean [ms] Min [ms] Max [ms] Relative
grep . *.* 7.5 ± 4.6 0.0 20.0 1.0
egrep . *.* 9.3 ± 5.2 0.0 30.4 1.2
ggrep . *.* 10.7 ± 6.8 0.0 36.3 1.4

At the moment, its implemented in a way so that it will sort the table by relative speed. This behaviour makes sense when you want to compare all aspects. But using hyperfine to promote a specific command would probably prefer to keep the sequence as one can set the desired one at the top or bottom.

What do you think is the best way of presenting the results?

In theory, it could also be decided with a flag, but not sure it's worth the added complexity compared to how this feature is not core to hyperfine.

@sharkdp
Copy link
Owner

sharkdp commented Jan 25, 2019

But using hyperfine to promote a specific command would probably prefer to keep the sequence as one can set the desired one at the top or bottom.

Yes, I think I'd prefer the commands to be listed in the original command-line argument order.

In theory, it could also be decided with a flag, but not sure it's worth the added complexity compared to how this feature is not core to hyperfine.

Hm, yes. That would be a export-specific option. I think I'd rather keep things simple for now.

@mathiasrw
Copy link
Author

Very well. Im on the case.

Actually, its really easy as I just need to avoid the sorting - but I have been having "fun" unfolding the wonders of (i)muteabillity in rust to make relative speed part of the export for the other formats. Great chance to learn :)

@sharkdp
Copy link
Owner

sharkdp commented Jan 26, 2019

Sounds good. Let me know if you need help :-)

@sharkdp
Copy link
Owner

sharkdp commented Jun 8, 2019

I went ahead and implemented this in #175

@sharkdp
Copy link
Owner

sharkdp commented Jun 8, 2019

Released in v1.6.0.

@mathiasrw
Copy link
Author

@sharkdp Thank you so much. Sorry about the stalling of the progress.

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 a pull request may close this issue.

2 participants