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

Use BufWriter when STDOUT is not a TTY #206

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

jasonrhansen
Copy link
Collaborator

This is an optimization to avoid line buffering when redirecting output.

Close #193

@jonhoo
Copy link
Owner

jonhoo commented Jan 6, 2021

My expectation would actually be that the standard library itself implemented this. And it seems like it's intended to, but it's a FIXME:
https://github.com/rust-lang/rust/blob/41601ef39442bca19bc6ac8ef72e5d89335e92d7/library/std/src/io/stdio.rs#L491-L493

At the moment, it seems like there is no way to get a handle to a STDOUT that isn't line buffered. That is.. unfortunate. The issue to watch appears to be rust-lang/rust#60673, with the fixing PR being rust-lang/rust#78515. There's also rust-lang/rust#58326 which would be an alternative approach.

Ultimately, this means that once std fixes this behavior, this PR will be unnecessary, and performance will immediately improve. Merging this PR will also not improve performance I don't think. Providing a mechanism specifically for writing to a file would help, though I'm a little hesitant to merge a change that complicates both the internal and external API when it will ultimately become unnecessary following a change to std...

@jasonrhansen
Copy link
Collaborator Author

It will be really nice when the standard library implements this, but I still believe this PR should help with performance. When you write more than a line to the line writer, it will write the buffer directly to the inner writer. This means if we wrap stdio in a BufWriter, we'll be writing larger chunks at a time to the underlying line writer and It will skip the line buffer.

Of course it will still call memrchr to find the last newline, but it will do this much less frequently since it won't be doing it for each of our small writes.

https://github.com/rust-lang/rust/blob/master/library/std/src/io/buffered/linewritershim.rs

    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        let newline_idx = match memchr::memrchr(b'\n', buf) {
            // If there are no new newlines (that is, if this write is less than
            // one line), just do a regular buffered write (which may flush if
            // we exceed the inner buffer's size)
            None => {
                self.flush_if_completed_line()?;
                return self.buffer.write(buf);
            }
            // Otherwise, arrange for the lines to be written directly to the
            // inner writer.
            Some(newline_idx) => newline_idx + 1,
        };

Once the standard library is updated to handle this, we can remove this workaround.

jonhoo
jonhoo previously approved these changes Jan 8, 2021
@jonhoo
Copy link
Owner

jonhoo commented Jan 8, 2021

Yeah, that's a good point. Happy to merge. The CI failure appears to be something broken in tarpaulin, not in our code.

@jonhoo
Copy link
Owner

jonhoo commented Jan 8, 2021

Oh, actually, before I merge, would you mind updating the changelog?

@jonhoo jonhoo merged commit 8e7b35c into jonhoo:master Jan 8, 2021
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.

Support output to files directly instead of just via output redirection
2 participants