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

*: use our own std::io::copy() implementation #290

Merged
merged 2 commits into from
Jul 6, 2020
Merged

*: use our own std::io::copy() implementation #290

merged 2 commits into from
Jul 6, 2020

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Jul 2, 2020

The std one uses an 8 KiB buffer. Use a larger buffer to amortize syscall overhead.

src/io.rs Outdated

use crate::errors::*;

/// This is like `std::io:copy()`, but uses a buffer larger than 8 KiB.
Copy link
Member

Choose a reason for hiding this comment

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

Let's say why we do that too. (It's in the commit message yes, but that stuff can get harder to find as code moves around)

I also looked at this for ostree a while ago...see ostreedev/ostree@16c31a9b5#diff-7833e28ecd7891cb43d466346f39acbdR828
And linked from there: https://github.com/coreutils/coreutils/blob/master/src/ioblksize.h

Of course both that and the coreutils one are pre-Meltdown/Spectre. And also pre-io-uring (which is going to dramatically change things like this).

Anyways seems fine as is, but it's worth thinking about e.g. whether we should argue for std to do this by default (although that'd be easier if there was a version that knew at least a lower bound for how much data we were going to be copying, which would help avoid allocating large buffers for small copies)

The std one uses an 8 KiB buffer.  Use a larger buffer to amortize syscall
overhead.
@bgilbert
Copy link
Contributor Author

bgilbert commented Jul 2, 2020

Updated. There's already an upstream bug: rust-lang/rust#49921

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

LGTM

@bgilbert bgilbert merged commit af34859 into coreos:master Jul 6, 2020
@bgilbert bgilbert deleted the copy branch July 6, 2020 19:50
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.

2 participants