Skip to content

cat: Improve performance of formatting. #7519

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 22, 2025
Merged

cat: Improve performance of formatting. #7519

merged 1 commit into from
Mar 22, 2025

Conversation

karlmcdowall
Copy link
Contributor

Issue #7518
Add a BufWriter over stdout when cat outputs any kind of formattted data. This improves performance considerably.

@karlmcdowall
Copy link
Contributor Author

Benchmarking with this change...

$ hyperfine "./target/release/cat.original -n ./shakespeare.txt" "./target/release/cat -n ./shakespeare.txt" "/usr/bin/cat -n ./shakespeare.txt"
Benchmark 1: ./target/release/cat.original -n ./shakespeare.txt
  Time (mean ± σ):     260.1 ms ±   6.0 ms    [User: 155.6 ms, System: 104.5 ms]
  Range (min … max):   252.5 ms … 274.1 ms    11 runs
 
Benchmark 2: ./target/release/cat -n ./shakespeare.txt
  Time (mean ± σ):      19.5 ms ±   0.7 ms    [User: 17.5 ms, System: 1.9 ms]
  Range (min … max):    17.9 ms …  23.1 ms    135 runs
 
Benchmark 3: /usr/bin/cat -n ./shakespeare.txt
  Time (mean ± σ):      12.3 ms ±   0.8 ms    [User: 10.6 ms, System: 1.7 ms]
  Range (min … max):    11.0 ms …  18.1 ms    229 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Summary
  /usr/bin/cat -n ./shakespeare.txt ran
    1.59 ± 0.12 times faster than ./target/release/cat -n ./shakespeare.txt
   21.20 ± 1.49 times faster than ./target/release/cat.original -n ./shakespeare.txt

So still not as fast as GNU, but a big improvement. I'll try some other things too, but I'd like to get this change mainlined as-is, and anything else I can come up with I'll do in a separate PR.

sylvestre
sylvestre reviewed Mar 21, 2025
View reviewed changes
src/uu/cat/src/cat.rs Outdated
@@ -511,7 +511,8 @@ fn write_lines<R: FdReadable>(
) -> CatResult<()> {
let mut in_buf = [0; 1024 * 31];
let stdout = io::stdout();
let mut writer = stdout.lock();
let stdout = stdout.lock();
let mut writer = BufWriter::with_capacity(1024 * 32, stdout);
Copy link
Contributor

Choose a reason for hiding this comment

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

please document these numbers

@sylvestre
Copy link
Contributor

kudos :)

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: cat/cat-buf.log. cat/cat-buf.log is passing on 'main'. Maybe you have to rebase?
GNU test failed: misc/stdbuf.log. misc/stdbuf.log is passing on 'main'. Maybe you have to rebase?

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/cat/cat-buf. tests/cat/cat-buf is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/tee is no longer failing!

@karlmcdowall
Copy link
Contributor Author

Hmm - cat-buf test is failing, I'll take a look...

@sylvestre sylvestre mentioned this pull request Mar 21, 2025
Merged
@karlmcdowall karlmcdowall force-pushed the cat_perf branch 3 times, most recently from 294fd55 to 3d1fd05 Compare March 21, 2025 20:15
@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Mar 21, 2025 •
edited
Loading

cat-buf fixed. I had to add some extra buffer-flushing. I re-ran my benchmark after this change and it's still looking good (numbers haven't changes significantly).
Should all be good now 🤞

@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/stdbuf is no longer failing!
Congrats! The gnu test tests/misc/tee is no longer failing!

@sylvestre
Copy link
Contributor

@karlmcdowall you closed it, was it on purpose ? :)

@karlmcdowall
Copy link
Contributor Author

Yeah! I wanted to check something related to the is_interactive flag in the code... I've managed to convince myself there isn't an issue there though, just some other potential optimizations to keep in mind for the future (which I'll write up at some point).

@karlmcdowall karlmcdowall reopened this Mar 21, 2025
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/stdbuf is no longer failing!

@karlmcdowall
cat: Improve performance of formatting.
c84ee0a 
Issue uutils#7518
Add a BufWriter over stdout when cat outputs any kind of formattted
data. This improves performance considerably.
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)

@sylvestre sylvestre merged commit b540e18 into uutils:main Mar 22, 2025
68 checks passed
@karlmcdowall karlmcdowall mentioned this pull request Apr 19, 2025
Closed
@BrewTestBot BrewTestBot mentioned this pull request May 24, 2025
Merged
@moonfruit moonfruit mentioned this pull request May 26, 2025
Closed
@karlmcdowall karlmcdowall deleted the cat_perf branch July 5, 2025 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylvestre sylvestre sylvestre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karlmcdowall @sylvestre