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

Implement --output={null,pipe,<FILE>} #509

Merged
merged 5 commits into from
May 17, 2022
Merged

Conversation

tavianator
Copy link
Contributor

Fixes #377.

@tavianator
Copy link
Contributor Author

TLDR: The new --output=pipe option redirects stdout through a pipe instead of to /dev/null. On Linux, it's backed by splice(), so its overhead very competitive.

Benchmarks

The first benchmark is cat large, which has its output handled by --output=....
The other benchmarks are baselines:

  • cat large >/dev/null is effectively --output=null
  • cat large | cat >/dev/null is effectively --output=pipe on non-Linux (without splice())
  • cat large | pv -q >/dev/null is effectively --output=pipe on Linux (with splice())

--output=null (the default)

Basically the same as >/dev/null:

Command Mean [ms] Min [ms] Max [ms] Relative
cat large 166.9 ± 2.0 160.6 168.7 1.00
cat large >/dev/null 167.5 ± 1.2 165.4 169.5 1.00 ± 0.01
cat large | cat >/dev/null 492.8 ± 14.5 468.6 511.9 2.95 ± 0.09
cat large | pv -q >/dev/null 347.2 ± 14.9 323.0 363.6 2.08 ± 0.09

--output=pipe

cat large is within 2-3% of | pv -q >/dev/null, which makes sense as both are using splice().
These methods are about 40% faster than | cat >/dev/null, which uses read()/write() through a pipe.

Command Mean [ms] Min [ms] Max [ms] Relative
cat large 357.1 ± 4.4 348.8 363.5 2.13 ± 0.03
cat large >/dev/null 167.8 ± 1.4 165.6 170.9 1.00
cat large | cat >/dev/null 474.3 ± 15.9 458.0 510.9 2.83 ± 0.10
cat large | pv -q >/dev/null 348.2 ± 9.3 335.9 363.0 2.08 ± 0.06

--output=/tmp/file

Writing to tmpfs is ~10% faster than writing to /dev/null through a pipe! But slower than splice(), which makes sense.

Command Mean [ms] Min [ms] Max [ms] Relative
cat large 436.0 ± 2.2 433.1 439.6 2.61 ± 0.03
cat large >/dev/null 166.9 ± 1.7 162.5 169.5 1.00
cat large | cat >/dev/null 475.5 ± 20.2 445.8 504.4 2.85 ± 0.12
cat large | pv -q >/dev/null 364.2 ± 24.2 319.6 399.8 2.18 ± 0.15

@sharkdp
Copy link
Owner

sharkdp commented May 15, 2022

This is awesome - thank you very much! Also for the benchmarks which clearly show that this is working correctly.

I have a couple of questions before I proceed with the review:

  • Should we add --output=terminal and then treat --show-output as an alias for --output=terminal?
  • Concerning the Linux-implementation of --output=pipe using splice: Could that possibly cause any confusion among users? Don't get me wrong - it's very cool that we can do this. But imagine someone is comparing benchmark results on Linux and MacOS and cannot explain the difference. I mean, we would expect OS-specific differences anyway in the implementation of a pipe. And the user would have to type --output=pipe explicitly. But I wonder if it's worth mentioning that in the documentation at least? Or do we want to add something like --output=pipe and --output=pipe_splice? (I'd rather not)

src/cli.rs Outdated Show resolved Hide resolved
@tavianator
Copy link
Contributor Author

This is awesome - thank you very much! Also for the benchmarks which clearly show that this is working correctly.

You're welcome!

I have a couple of questions before I proceed with the review:

  • Should we add --output=terminal and then treat --show-output as an alias for --output=terminal?

Yeah I think that makes sense for consistency. Maybe --output=show/shared/inherit or something? It's not necessarily a terminal if hyperfine is redirected.

I also think we should eventually add --output=pty (what I described here), so that benchmarks will think isatty(stdout) without flooding the current terminal with output.

Which reminds me, I think --output=<FILE> should require a path separator in <FILE>, so that we can add --output=word backwards compatibly.

  • Concerning the Linux-implementation of --output=pipe using splice: Could that possibly cause any confusion among users? Don't get me wrong - it's very cool that we can do this. But imagine someone is comparing benchmark results on Linux and MacOS and cannot explain the difference. I mean, we would expect OS-specific differences anyway in the implementation of a pipe. And the user would have to type --output=pipe explicitly. But I wonder if it's worth mentioning that in the documentation at least? Or do we want to add something like --output=pipe and --output=pipe_splice? (I'd rather not)

I don't see the benefit of a separate --output=pipe_splice -- if it's available, you should pretty much always use it over pipe, so why not make it the default? There's so much that's different between macOS and Linux anyway that a direct comparison is probably not informative IMO.

@sharkdp
Copy link
Owner

sharkdp commented May 16, 2022

Yeah I think that makes sense for consistency. Maybe --output=show/shared/inherit or something? It's not necessarily a terminal if hyperfine is redirected.

Good point. I like --output=inherit which seems to be common terminology (e.g. being used in Rust or Python).

I also think we should eventually add --output=pty (what I described here), so that benchmarks will think isatty(stdout) without flooding the current terminal with output.

I remember that comment. Yes, absolutely agreed.

Which reminds me, I think --output=<FILE> should require a path separator in <FILE>, so that we can add --output=word backwards compatibly.

👍 I was thinking about this edge case ("what if a user wants to print to a file called null") but didn't post a comment because it seemed too nitpicky. But simply disallowing paths without a separator sounds like a good idea.

I don't see the benefit of a separate --output=pipe_splice -- if it's available, you should pretty much always use it over pipe, so why not make it the default? There's so much that's different between macOS and Linux anyway that a direct comparison is probably not informative IMO.

Okay.

@tavianator
Copy link
Contributor Author

Yeah I think that makes sense for consistency. Maybe --output=show/shared/inherit or something? It's not necessarily a terminal if hyperfine is redirected.

Good point. I like --output=inherit which seems to be common terminology (e.g. being used in Rust or Python).

Done.

Which reminds me, I think --output=<FILE> should require a path separator in <FILE>, so that we can add --output=word backwards compatibly.

+1 I was thinking about this edge case ("what if a user wants to print to a file called null") but didn't post a comment because it seemed too nitpicky. But simply disallowing paths without a separator sounds like a good idea.

Done.

src/timer/mod.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
src/options.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
src/timer/mod.rs Outdated Show resolved Hide resolved
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 3df63ba into sharkdp:master May 17, 2022
@tavianator tavianator deleted the output-null branch May 18, 2022 14:16
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.

consider an option to capture stdout without printing it
2 participants