Skip to content

head: rework handling of non-seekable files #7438

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 2 commits into from
Mar 18, 2025
Merged

head: rework handling of non-seekable files #7438

merged 2 commits into from
Mar 18, 2025

Conversation

karlmcdowall
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link

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 GitHub Actions
Copy link

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

Changes give large performance improvement.

Can you attach a benchmark to show the improvement ?

@github-actions GitHub Actions
Copy link

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

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 GitHub Actions
Copy link

GNU testsuite comparison:

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

@github-actions GitHub Actions
Copy link

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

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 GitHub Actions
Copy link

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 GitHub Actions
Copy link

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 GitHub Actions
Copy link

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

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 GitHub Actions
Copy link

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