Skip to content

od: fix for issue #7666 #7776

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
Apr 19, 2025
Merged

od: fix for issue #7666 #7776

merged 1 commit into from
Apr 19, 2025

Conversation

karlmcdowall
Copy link
Contributor

@karlmcdowall karlmcdowall commented Apr 18, 2025 •
edited by cakebaker
Loading

Fixes #7666

For `od` utility, if client specifies `-N` maximum bytes to be read
then ensure stdin is left pointing to the next byte when `od` exits.
To do this...
 - Bypass standard buffering on stdin.
 - Instantiate BufReader further up the stack to maintain performance.

@karlmcdowall karlmcdowall mentioned this pull request Apr 18, 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/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/od/od-N is no longer failing!

@karlmcdowall
Copy link
Contributor Author

Can confirm that the performance does indeed regress quite a bit...

 hyperfine -L od ./target/release/od,./target/release/od.original "{od} <./shakespeare.txt"
Benchmark 1: ./target/release/od <./shakespeare.txt
  Time (mean ± σ):      1.260 s ±  0.029 s    [User: 0.819 s, System: 0.441 s]
  Range (min … max):    1.216 s …  1.299 s    10 runs
 
Benchmark 2: ./target/release/od.original <./shakespeare.txt
  Time (mean ± σ):     777.7 ms ±  18.6 ms    [User: 605.8 ms, System: 171.2 ms]
  Range (min … max):   757.1 ms … 819.8 ms    10 runs
 
Summary
  ./target/release/od.original <./shakespeare.txt ran
    1.62 ± 0.05 times faster than ./target/release/od <./shakespeare.txt

I have an idea how to fix this without a lot of rework. Will post when I have time to look at it.

@karlmcdowall
Copy link
Contributor Author

Latest code doesn't regress performance...

$ hyperfine -L od ./target/release/od,./target/release/od.original "{od} <./shakespeare.txt"
Benchmark 1: ./target/release/od <./shakespeare.txt
  Time (mean ± σ):     771.3 ms ±  15.3 ms    [User: 599.7 ms, System: 171.0 ms]
  Range (min … max):   759.1 ms … 806.6 ms    10 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.
 
Benchmark 2: ./target/release/od.original <./shakespeare.txt
  Time (mean ± σ):     770.9 ms ±   5.4 ms    [User: 597.3 ms, System: 173.3 ms]
  Range (min … max):   762.3 ms … 778.7 ms    10 runs
 
Summary
  ./target/release/od.original <./shakespeare.txt ran
    1.00 ± 0.02 times faster than ./target/release/od <./shakespeare.txt

👍

@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)
Congrats! The gnu test tests/od/od-N is no longer failing!

@karlmcdowall karlmcdowall changed the title od: draft fix for issue #7666 od: fix for issue #7666 Apr 19, 2025
@karlmcdowall karlmcdowall marked this pull request as ready for review April 19, 2025 01:44
@karlmcdowall karlmcdowall force-pushed the od_7666 branch 2 times, most recently from 117f1c2 to 2640628 Compare April 19, 2025 02:42
@karlmcdowall
od: Ensure stdin is left in the correct state
b692fad 
Fixes issue uutils#7666
For `od` utility, if client specifies `-N` maximum bytes to be read
then ensure stdin is left pointing to the next byte when `od` exits.
To do this...
 - Bypass standard buffering on stdin.
 - Instantiate BufReader further up the stack to maintain performance.
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/od/od-N is no longer failing!

@sylvestre sylvestre merged commit e7f33f5 into uutils:main Apr 19, 2025
70 checks passed
@BrewTestBot BrewTestBot mentioned this pull request May 24, 2025
Merged
@moonfruit moonfruit mentioned this pull request May 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

od: fails to leave remaining bytes of input file available for subsequent commands
2 participants
@karlmcdowall @sylvestre