Skip to content

head: rework handling of non-seekable files#7438

Merged
RenjiSann merged 2 commits intouutils:mainfrom
karlmcdowall:head_perf2
Mar 18, 2025
Merged

head: rework handling of non-seekable files#7438
RenjiSann merged 2 commits intouutils:mainfrom
karlmcdowall:head_perf2

Conversation

@karlmcdowall
Copy link
Contributor

@karlmcdowall karlmcdowall commented Mar 12, 2025

Rework logic for handling all-but-last-lines and all-but-last-bytes for non-seekable files. Changes give large performance improvement.

@github-actions
Copy link

github-actions bot commented Mar 12, 2025

GNU testsuite comparison:

GNU test failed: tests/head/head-write-error. tests/head/head-write-error is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

@karlmcdowall karlmcdowall force-pushed the head_perf2 branch 2 times, most recently from 7de319d to 1af6e51 Compare March 12, 2025 20:09
@github-actions
Copy link

github-actions bot commented Mar 12, 2025

GNU testsuite comparison:

GNU test failed: tests/misc/tee. tests/misc/tee 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)

@RenjiSann
Copy link
Collaborator

RenjiSann commented Mar 13, 2025

Changes give large performance improvement.

Can you attach a benchmark to show the improvement ?

@github-actions
Copy link

github-actions bot commented Mar 14, 2025

GNU testsuite comparison:

GNU test failed: tests/misc/tee. tests/misc/tee 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)

@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Mar 14, 2025

Changes give large performance improvement.

Can you attach a benchmark to show the improvement ?

The performance improvements are seen when the input comes in via a pipe. Comparing current head with head_improved which contains my changes...

hyperfine "cat ./shakespeare.txt | ./target/release/head -c -100000" "cat ./shakespeare.txt | ./target/release/head_improved -c -100000"
Summary
  cat ./shakespeare.txt | ./target/release/head_improved -c -100000 ran
   17.93 ± 2.56 times faster than cat ./shakespeare.txt | ./target/release/head -c -100000


hyperfine "cat ./shakespeare.txt | ./target/release/head -n -100000" "cat ./shakespeare.txt | ./target/release/head_improved -n -100000"
Summary
  cat ./shakespeare.txt | ./target/release/head_improved -n -100000 ran
   24.38 ± 2.60 times faster than cat ./shakespeare.txt | ./target/release/head -n -100000

Also should note though that I want to add a bit more test code before anyone reviews these changes. Give me another day or so please :)

@karlmcdowall karlmcdowall force-pushed the head_perf2 branch 2 times, most recently from 1e2de83 to ebbac05 Compare March 15, 2025 22:30
@github-actions
Copy link

github-actions bot commented Mar 15, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Mar 17, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Mar 17, 2025

In addition to the main changes here, I also made a few changes to add/remove buffering (BufReader/BufWriter) through the head.rs file - I've remove some buffering that was redundant and added it where I think it's helpful. Here are the performance numbers before/after my changes on my machine (running Linux) to confirm that nothing has regressed...

Command Time Before (ms) Time After (ms) Improvement Ratio
head -n 100000 ./shakespeare.txt 3.6 3.0 1.19
head -n -100000 ./shakespeare.txt 3.9 3.9 1
head -c 100000 ./shakespeare.txt 1.0 1.0 1
head -c -100000 ./shakespeare.txt 1.2 1.2 1
cat ./shakespeare.txt | head -n 100000 4.0 3.5 1.15
cat ./shakespeare.txt | head -n -100000 133.5 5.7 23
cat ./shakespeare.txt | head -c 100000 1.4 1.4 1
cat ./shakespeare.txt | head -c -100000 75.5 4.0 19

So, no regressions, some reasonable improvements (~15%) for the ... -n 100000 cases, and some really big improvements for the cat ... -100000 cases.

If there are any other usecases people think I should run then please let me know.

@karlmcdowall karlmcdowall marked this pull request as ready for review March 17, 2025 01:56
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

GNU testsuite comparison:

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

@karlmcdowall karlmcdowall reopened this Mar 17, 2025
@karlmcdowall karlmcdowall mentioned this pull request Mar 17, 2025
Closed
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

GNU testsuite comparison:

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

@karlmcdowall
head: rework handling of non-seekable files
2987531 
Fix issue uutils#7372
Rework logic for handling all-but-last-lines and all-but-last-bytes
for non-seekable files. Changes give large performance improvement.
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

GNU testsuite comparison:

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

RenjiSann
RenjiSann reviewed Mar 18, 2025
View reviewed changes
RenjiSann
RenjiSann requested changes Mar 18, 2025
View reviewed changes
Copy link
Collaborator

@RenjiSann RenjiSann left a comment

Choose a reason for hiding this comment

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

Appart from the typo. this looks good ^^
It's nice to see such performance improvement 🚀

@karlmcdowall @RenjiSann
Update src/uu/head/src/take.rs
e1275f4 
Co-authored-by: Dorian Péron <72708393+RenjiSann@users.noreply.github.com>
@karlmcdowall
Copy link
Contributor Author

karlmcdowall commented Mar 18, 2025

Appart from the typo. this looks good ^^ It's nice to see such performance improvement 🚀

Thanks for the review!

@karlmcdowall karlmcdowall requested a review from RenjiSann March 18, 2025 15:29
@github-actions
Copy link

github-actions bot commented Mar 18, 2025

GNU testsuite comparison:

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

@RenjiSann RenjiSann merged commit 2e3da88 into uutils:main Mar 18, 2025
67 checks passed
@BrewTestBot BrewTestBot mentioned this pull request May 24, 2025
Merged
@moonfruit moonfruit mentioned this pull request May 26, 2025
Closed
@karlmcdowall karlmcdowall deleted the head_perf2 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

@RenjiSann RenjiSann RenjiSann approved these changes

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 @RenjiSann