Change to non-line buffered output if output is not a TTY #64861
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
tbu-
force-pushed
the
pr_stdout_nonlinebuffered
branch
from
8baed87
to
612524d
Compare
This matches glibc behavior. This is determined using the `isatty` function on Unixes, and not attempted at all for other operating systems. Fixes rust-lang#60673.
tbu-
force-pushed
the
pr_stdout_nonlinebuffered
branch
from
612524d
to
fb48c55
Compare
|
I get a 4x speed up for the worst case of the old implementation: fn main() {
for _ in 0..(1<<20) {
println!("y");
}
}The old program is doing 1048576 Changing the program to use an infinite loop, I get a throughput of 3MB/s vs 20MB/s. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
An interesting question here is what we should do This doesn't output the |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
tbu-
force-pushed
the
pr_stdout_nonlinebuffered
branch
from
2436735
to
5279048
Compare
KodrAus
reviewed
Oct 10, 2019
| StdioReader::Fake => Ok(0), | ||
| } | ||
| } | ||
| #[inline] |
| pub fn should_be_line_buffered(&self) -> bool { | ||
| // FIXME: Fill in. I don't know how to check whether output is | ||
| // redirected on Windows. | ||
| true |
There was a problem hiding this comment.
For UWP I think we can also use GetConsoleMode.
| pub fn should_be_line_buffered(&self) -> bool { | ||
| // FIXME: Fill in. I don't know how to check whether output is | ||
| // redirected on Windows. | ||
| true |
There was a problem hiding this comment.
On Windows there's a GetConsoleMode function you can call. It's used in the atty library's Windows implementation. It looks like that crate also has some more complex heuristics for Windows but those might not be so important here.
| /// A writer that can either be line-buffered, buffered, unbuffered or fake. | ||
| /// | ||
| /// If it is fake, all outputs just succeed and are dropped silently. | ||
| enum StdioWriter<W: Write> { |
There was a problem hiding this comment.
There was a comment in the original FIXME suggesting we should call flush when dropping the handle if we're panicking. Did you have any thoughts on this @tbu-?
bluss
reviewed
Oct 15, 2019
| StdioWriter::LineBuffered(i) => i.write_vectored(bufs), | ||
| StdioWriter::Buffered(i) => i.write_vectored(bufs), | ||
| StdioWriter::Unbuffered(i) => i.write_vectored(bufs), | ||
| StdioWriter::Fake => Ok(bufs.iter().map(|b| b.len()).sum()), |
There was a problem hiding this comment.
This sum could probably wrap around, seems like it should saturate instead (not sure what's reasonable), or return the first non-empty len.
Dylan-DPC-zz
added
S-waiting-on-author
|
@tbu- waiting for you to answer the review |
|
Ping from Triage: @tbu- any updates? |
|
Too little time right now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This matches glibc behavior.
This is determined using the
isattyfunction on Unixes, and notattempted at all for other operating systems.
Fixes #60673.