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 unbuffered I/O for faster printing to stdout, switch to Rustls #187

Merged
merged 3 commits into from
May 26, 2021

Conversation

sondr3
Copy link
Contributor

@sondr3 sondr3 commented May 9, 2021

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 of outfieldr ran, but you can also see a comparison I posted there.

Benchmark #1: ./target/release/tldr tar
  Time (mean ± σ):      29.3 ms ±   6.2 ms    [User: 1.7 ms, System: 4.3 ms]
  Range (min … max):    24.3 ms …  50.1 ms    22 runs

tests/tar-markdown.expected Outdated Show resolved Hide resolved
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, great idea

Here is what I measured on my machine

image

src/formatter.rs Show resolved Hide resolved
src/formatter.rs Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@sondr3
Copy link
Contributor Author

sondr3 commented May 13, 2021

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.

Copy link
Collaborator

@niklasmohrin niklasmohrin left a 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... :)

src/formatter.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@sondr3
Copy link
Contributor Author

sondr3 commented May 16, 2021

I actually started converting the unwraps to proper errors, but figured it was out of the scope of this PR. Happy to fix though, it may take a few days as we are celebrating our Independence Day tomorrow 😄

@sondr3 sondr3 force-pushed the performance-fixes branch 3 times, most recently from 9a7784e to 9b3cfaf Compare May 20, 2021 10:29
@sondr3
Copy link
Contributor Author

sondr3 commented May 20, 2021

I think I managed to fix the requested changes, this is ready for another round of reviews.

Copy link
Collaborator

@niklasmohrin niklasmohrin left a 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 👍

src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@sondr3
Copy link
Contributor Author

sondr3 commented May 21, 2021

Done 😃

@dbrgn
Copy link
Collaborator

dbrgn commented May 21, 2021

Thanks for the first round of reviews, @niklasmohrin. I'll try to take a look this weekend!

Copy link
Collaborator

@dbrgn dbrgn left a 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.

src/error.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@dbrgn dbrgn 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 @sondr3!

@dbrgn dbrgn changed the title Use unbuffered IO for faster printing to stdout, static linking to OpenSSL Use unbuffered I/O for faster printing to stdout, switch to Rustls May 26, 2021
@dbrgn dbrgn merged commit 767b62d into tealdeer-rs:master May 26, 2021
@sondr3 sondr3 deleted the performance-fixes branch May 26, 2021 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants