Skip to content

Use is_write_vectored to optimize the write_vectored implementation for BufWriter #78768

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

Merged
merged 4 commits into from
Dec 9, 2020

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 5, 2020

In case when the underlying writer does not have an efficient implementation write_vectored, the present implementation of
write_vectored for BufWriter may still forward vectored writes directly to the writer depending on the total length of the data. This misses the advantage of buffering, as the actually written slice may be small.

Provide an alternative code path for the non-vectored case, where the slices passed to BufWriter are coalesced in the buffer before being flushed to the underlying writer with plain write calls. The buffer is only bypassed if an individual slice's length is at least as large as the buffer.

Remove a FIXME comment referring to #72919 as the issue has been closed with an explanation provided.

@rust-highfive
Copy link
Contributor

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 5, 2020

Did you see #78551? That PR is also making changes to the way BufWriter works. I didn't look at your changes yet, but judging by the PR title there's a lot of overlap.

@m-ou-se m-ou-se added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 5, 2020
@mzabaluev
Copy link
Contributor Author

Did you see #78551? That PR is also making changes to the way BufWriter works.

Thanks @m-ou-se, I was unaware of it!
I should review that PR carefully to see if this one has anything on it.

One difference I see is that it may try to flush the buffer multiple times while going over the slices. I think this is a violation of the contract of write_vectored.

@mzabaluev
Copy link
Contributor Author

it may try to flush the buffer multiple times

I was wrong, #78551 does better. It's also a more extensive change, so I will try to incorporate the part of it that applies to vectored_write to get a smaller piece to review.

If the underlying writer does not support efficient vectored output,
do it differently: always try to coalesce the slices in the buffer
until one comes that does not fit entirely. Flush the buffer before
the first slice if needed.
BufWriter provides an efficient implementation of
write_vectored also when the underlying writer does not
support vectored writes.
Now that BufWriter always claims to support vectored writes,
look through it at the wrapped writer to decide whether to
use vectored writes for LineWriter.
Do what write does and optimize for the most likely case:
slices are much smaller than the buffer. If a slice does not fit
completely in the remaining capacity of the buffer, it is left out
rather than buffered partially. Special treatment is only left for
oversized slices that are written directly to the underlying writer.
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@JohnCSimon
Copy link
Member

@mzabaluev - Ping from triage. Can you please post the status of this PR?

@mzabaluev
Copy link
Contributor Author

mzabaluev commented Dec 7, 2020

@JohnCSimon It's ready for review, I believe. I have made changes that reduce branching while optimizing for the most likely case: the slices passed to write_vectored are much smaller than the buffer capacity.

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 8, 2020
@Dylan-DPC-zz
Copy link

@cramertj this is ready for review

@cramertj
Copy link
Member

cramertj commented Dec 8, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 8, 2020

📌 Commit 674dd62 has been approved by cramertj

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2020
@bors
Copy link
Collaborator

bors commented Dec 9, 2020

⌛ Testing commit 674dd62 with merge 2c56ea3...

@bors
Copy link
Collaborator

bors commented Dec 9, 2020

☀️ Test successful - checks-actions
Approved by: cramertj
Pushing 2c56ea3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2020
@bors bors merged commit 2c56ea3 into rust-lang:master Dec 9, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants