Significantly improve performance by using a buffered writer #3101
Conversation
Before ``` Benchmark 1: seq 10000000 | bat Time (mean ± σ): 6.235 s ± 0.052 s [User: 3.664 s, System: 2.714 s] Range (min … max): 6.172 s … 6.355 s 10 runs ``` After ``` Benchmark 1: seq 10000000 | ./target/release/bat Time (mean ± σ): 215.9 ms ± 5.1 ms [User: 275.4 ms, System: 38.8 ms] Range (min … max): 210.3 ms … 224.9 ms 10 runs ``` Using `less` for comparison ``` Benchmark 1: seq 10000000 | less Time (mean ± σ): 637.3 ms ± 43.3 ms [User: 642.1 ms, System: 95.6 ms] Range (min … max): 584.5 ms … 700.1 ms 10 runs ``` And raw ``` Benchmark 1: seq 10000000 Time (mean ± σ): 63.1 ms ± 1.3 ms [User: 57.1 ms, System: 5.9 ms] Range (min … max): 62.1 ms … 66.0 ms 10 runs ``` Signed-off-by: Mohammad AlSaleh <[email protected]>
|
Those numbers look very nice 👍 I skimmed over the diff which is quite big. Is it possible to split this up into smaller commits/PRs to make it easier to review/handle? |
|
Didn't have time to look into the code, but didn't we experiment with this a while ago and decided that we can't use a buffered writer, because we need to support interactive use cases where you run |
| let mut writer = match output_buffer { | ||
| Some(buf) => OutputHandle::FmtWrite(buf), | ||
| None => OutputHandle::IoWrite(output_type.handle()?), | ||
| None => { | ||
| OutputHandle::IoWrite(BufWriter::with_capacity(BUF_W_SZ, output_type.handle()?)) |
There was a problem hiding this comment.
That might very well be the case. If so, it would be good to add a comment e.g. here that explains that. So that anyone that gets the same idea will find the comment when they are about to change the code.
And/or add a regression test that tests this use case so that CI does not become green.
bat supports interactive use by opening without file arguments or with `-`. This uses the terminal stdin for input. This won't work with a buffered writer if the writer is not flushed after each read line from input. So this commit enables such flushing behavior if stdin is detected as terminal input, while retaining the benefits of using a buffered writer otherwise. Note that `Input::from_stdin()` from the `pretty_printer` API will assume input is terminal to retain old (unbuffered) behavior while the API is kept the same. Signed-off-by: Mohammad AlSaleh <[email protected]>
|
@sharkdp |
That wouldn't account for other programs piping to bat: If it's possible to flush at the end of every input line regardless of the source, I would be happy with that compromise. We can always optimize it further in the future to add special exceptions for cases such as "the input source is a regular file." |
Before
After
Using
lessfor comparisonAnd raw