-
Notifications
You must be signed in to change notification settings - Fork 128
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 unbuffered I/O for faster printing to stdout, switch to Rustls #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated according to the reviews, I'm on a different computer so can't run the benchmark again but there shouldn't be a difference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having so many unwrap
calls looks a bit wrong, but it is equivalent to what we had before, so... :)
I actually started converting the |
9a7784e
to
9b3cfaf
Compare
I think I managed to fix the requested changes, this is ready for another round of reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Only thing I can really point out is that you wrote the helpful error message for writing to stdout, but discard it at the caller. I think it's better to propagate this information to the user (in the unlikely case that this goes wrong)
Other than that, thank you for your contribution 👍
Done 😃 |
Thanks for the first round of reviews, @niklasmohrin. I'll try to take a look this weekend! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks @sondr3!
The switch to RusTLS causes an increase in size of roughly 1.8 MiB (pre-strip). After stripping, the size increase is 868 KiB, which I think is fine given that OpenSSL is gone (which makes cross-compilation and static builds so much simpler).
Regarding the performance, I did a hyperfine comparison. Plain tldr
which just prints the help page:
Before
Time (mean ± σ): 12.1 ms ± 0.8 ms [User: 1.6 ms, System: 2.7 ms]
Range (min … max): 10.4 ms … 14.1 ms 45 runs
After
Time (mean ± σ): 11.0 ms ± 0.7 ms [User: 1.7 ms, System: 2.0 ms]
Range (min … max): 10.1 ms … 13.2 ms 33 runs
And with tldr tar
:
Before
Time (mean ± σ): 14.0 ms ± 1.1 ms [User: 1.5 ms, System: 3.2 ms]
Range (min … max): 12.2 ms … 17.3 ms 32 runs
After
Time (mean ± σ): 13.8 ms ± 0.8 ms [User: 1.5 ms, System: 2.6 ms]
Range (min … max): 12.5 ms … 15.5 ms 39 runs
Looks like a small but clear improvement. Depending on the printed page and I/O speed, it might make a bigger difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @sondr3!
rustls
over dynamically linkedopenssl
.Compared to the benchmark in #129 from the first post (ran
hyperfine --prepare 'sync; echo 3 | sudo tee /proc/sys/vm/drop_caches' './target/release/tldr tar'
). I don't know the exact command that the author ofoutfieldr
ran, but you can also see a comparison I posted there.