Let io::copy reuse BufWriter buffers #78641
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
the8472
force-pushed
the
buffered-copy
branch
from
9f4919c to
8623f04
Compare
jyn514
added
the
T-libs-api
|
☔ The latest upstream changes (presumably #75272) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
shepmaster
added
S-blocked
the8472
force-pushed
the
buffered-copy
branch
from
8623f04 to
221c8fc
Compare
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-review
|
The resulting code feels a bit gnarly but I didn't want to duplicate the read-write loop because it contains all that unsafe code around uninitialized memory. If it's accaptable to duplicate that code then the generic and the BufWriter-specific version could individually be made cleaner. |
This comment has been minimized.
This comment has been minimized.
rustbot
removed
the
S-blocked
|
I'll hold off fixing the nits until I get a higher-level review. |
This comment has been minimized.
This comment has been minimized.
|
I'ven't been able to give this the time it deserves. r? @sfackler |
| @@ -56,12 +56,47 @@ where | |||
|
|
|||
| /// The general read-write-loop implementation of | |||
| /// `io::copy` that is used when specializations are not available or not applicable. | |||
| pub(crate) fn generic_copy<R: ?Sized, W: ?Sized>(reader: &mut R, writer: &mut W) -> io::Result<u64> | |||
| pub(crate) fn generic_copy<R: ?Sized, W: ?Sized>(reader: &mut R, writer: &mut W) -> Result<u64> | |||
There was a problem hiding this comment.
It seems strange for there to be specialization in the function used "when specializations are not available or not applicable".
There was a problem hiding this comment.
That refers to another set of specializations. I'll try to reword it.
|
This looks good, thanks! @bors r+ |
|
📌 Commit 4105506 has been approved by |
bors
added
S-waiting-on-bors
| // Safety: The initializer contract guarantees that either it or `read` | ||
| // will have initialized these bytes. And we just checked that the number | ||
| // of bytes is within the buffer capacity. | ||
| unsafe { buf.set_len(buf.len() + bytes_read) }; |
There was a problem hiding this comment.
Huh, is std being built with polonious?
There was a problem hiding this comment.
Not sure what is causing surprise here. The x.foo(x.bar())? Isn't that just two-phase borrows?
|
⌛ Testing commit 4105506 with merge 8bd506e0f39d10dcc588ae469c9c6c73d0c568c6... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
The macOS builder is still running and showing new logs. |
|
@bors r+ retry |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 4105506 has been approved by |
bors
added
S-waiting-on-bors
|
Oops, should have done @bors r=sfackler |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit 4105506 has been approved by |
Let io::copy reuse BufWriter buffers This optimization will allow users to implicitly set the buffer size for io::copy by wrapping the writer into a `BufWriter` if the default block size is insufficient, which should fix rust-lang#49921 Due to min_specialization limitations this approach only works with `BufWriter` but not for `BufReader<R>` since `R` is unconstrained and thus the necessary specialization on `R: Read` is not always applicable. Once specialization becomes more powerful this optimization could be extended to look at the reader and writer side and use whichever buffer is larger.
…as-schievink Rollup of 12 pull requests Successful merges: - rust-lang#78641 (Let io::copy reuse BufWriter buffers) - rust-lang#79291 (Add error message for private fn) - rust-lang#81364 (Improve `rustc_mir_build::matches` docs) - rust-lang#81387 (Move some tests to more reasonable directories - 3) - rust-lang#81463 (Rename NLL* to Nll* accordingly to C-CASE) - rust-lang#81504 (Suggest accessing field when appropriate) - rust-lang#81529 (Fix invalid camel case suggestion involving unicode idents) - rust-lang#81536 (Indicate both start and end of pass RSS in time-passes output) - rust-lang#81592 (Rustdoc UI fixes) - rust-lang#81594 (Avoid building LLVM just for llvm-dwp) - rust-lang#81598 (Fix calling convention for CRT startup) - rust-lang#81618 (Sync rustc_codegen_cranelift) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
|
|
||
| writer.flush_buf()?; |
There was a problem hiding this comment.
I think putting this in an else branch will be clearer, if the previous conditions are hit all will either continue or return, it does not reach this part.
This optimization will allow users to implicitly set the buffer size for io::copy by wrapping the writer into a
BufWriterif the default block size is insufficient, which should fix #49921Due to min_specialization limitations this approach only works with
BufWriterbut not forBufReader<R>sinceRis unconstrained and thus the necessary specialization onR: Readis not always applicable. Once specialization becomes more powerful this optimization could be extended to look at the reader and writer side and use whichever buffer is larger.