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

dd: fix output issues #3610

Merged
merged 7 commits into from
Jun 24, 2022
Merged

dd: fix output issues #3610

merged 7 commits into from
Jun 24, 2022

Conversation

arcuru
Copy link
Contributor

@arcuru arcuru commented Jun 9, 2022

This change fixes multiple issues with the output of dd, that were easiest to complete in a single PR.

  1. Correctly respect SIGUSR1 signals, by marking the setting as 'seen' once we have responded to it.
  2. Fix a race condition in the output by moving all output into a single thread.
  3. Fixes the bad combination of an existing 'progress' line and final output which can result in printing both on the same line.
  4. Moves the check for whether output is needed (only every 1 second) into the main thread to reduce thread-to-thread communication.

The performance uplift of fixing the overzealous thread communication is significant for small blocksizes, and stacks with the buffer changes in #3600.

  bs=4k count=1000000 bs=1M count=20000 bs=1G count=10
Coreutils dd 2.14s 1.98s 5.98s
Coreutils dd (Reused buffer #3600) 1.99s 1.60s 1.95s
Coreutils dd (this change, output updates) 1.29s 1.79s 5.96s
Coreutils dd (buffer+output) 1.15s 1.42s 1.90s
GNU dd 1.15s 1.07s 1.13s

@sylvestre sylvestre force-pushed the dd-progress-report branch from a2b0180 to f1e85d5 Compare June 10, 2022 08:06
Copy link
Collaborator

@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

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

Cool

src/uu/dd/src/progress.rs Show resolved Hide resolved
@sylvestre sylvestre force-pushed the dd-progress-report branch from f1e85d5 to a93aa9a Compare June 12, 2022 14:03
@sylvestre
Copy link
Contributor

Looks like my manual merge didn't work, could you please fix it? sorry :(

@arcuru
Copy link
Contributor Author

arcuru commented Jun 15, 2022

No problem @sylvestre, I've merged the current main branch into this one.

@sylvestre
Copy link
Contributor

thanks!

@sylvestre
Copy link
Contributor

@patricksjackson did you push it?

@arcuru
Copy link
Contributor Author

arcuru commented Jun 21, 2022

I misunderstood, and I hadn't realized that the merge broke the tests.

I've fixed the tests that were broken due to the merge and have pushed the new changes.

@sylvestre sylvestre merged commit 78a77c4 into uutils:main Jun 24, 2022
niyaznigmatullin pushed a commit to niyaznigmatullin/coreutils that referenced this pull request Jun 25, 2022
@sylvestre
Copy link
Contributor

this change has been documented here:
https://jackson.dev/post/rust-coreutils-dd/

(i just saw it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants