-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add Read/Write::can_read/write_vectored #67841
Conversation
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
cc @nikomatsakis this is part of what we talked about when discussing buffer initialization and vectored IO. |
src/libstd/io/mod.rs
Outdated
/// If a `Read`er does not override the default `read_vectored` | ||
/// implementation, code using it may want to avoid the method all together | ||
/// and coalesce writes into a single buffer for higher performance. | ||
#[unstable(feature = "can_vector", issue = "none")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fill in the issue assuming people are happy with this approach.
Why not just transparently merge the buffers? At this point the support for vectored I/O is becoming complicated enough that I think a broader discussion is worthwhile (i.e. an RFC). Also the name is not great. |
Because user code has more flexibility around how to merge those buffers.
Sure - definitely open to better naming. |
This seems reasonable to me.
Perhaps something along the lines of the runtime CPU checks? let strategy = if writer.is_write_vectored_optimized() {
WriteStrategy::Queue
} else {
WriteStrategy::Flatten
}; Also not a really great name, but the |
@sfackler any updates? |
|
This comment has been minimized.
This comment has been minimized.
I'd be on board with |
Renamed, and added a tracking issue |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #70062) made this pull request unmergeable. Please resolve the merge conflicts. |
@sfackler you have a merge commit. Can you unmerge it and rebase instead? thanks |
Fixed |
@KodrAus this is ready for review |
☔ The latest upstream changes (presumably #70136) made this pull request unmergeable. Please resolve the merge conflicts. |
📌 Commit 5d8fe1c has been approved by |
⌛ Testing commit 5d8fe1c with merge dd0bfd08f17cebeefea8fe2ae716edc78d3bd088... |
@bors retry (yield) |
⌛ Testing commit 5d8fe1c with merge c2f3b75ad1ed2899a39346f3cfe57930fc8f914a... |
💔 Test failed - checks-azure |
@bors r=Amaneiu |
📌 Commit b00afb5 has been approved by |
⌛ Testing commit b00afb5 with merge 80ba2ba544868e6ae3dc83dd5c18cb6764c905da... |
💔 Test failed - checks-azure |
@bors r=Amaneiu |
📌 Commit c68f23f has been approved by |
Rollup of 7 pull requests Successful merges: - rust-lang#67841 (Add Read/Write::can_read/write_vectored) - rust-lang#71524 (Minimize parameter of coerce_borrowed_pointer()) - rust-lang#71558 (Cleanup and document `-Z tls-model` ) - rust-lang#71578 (linkchecker: fix typo in main.rs) - rust-lang#71596 (Fix broken link in `QPath` documentation) - rust-lang#71604 (make recursive-zst test unleashed) - rust-lang#71605 (No need to whitelist E0750 anymore) Failed merges: r? @ghost
When working with an arbitrary reader or writer, code that uses vectored
operations may end up being slower than code that copies into a single
buffer when the underlying reader or writer doesn't actually support
vectored operations. These new methods allow you to ask the reader or
witer up front if vectored operations are efficiently supported.
Currently, you have to use some heuristics to guess by e.g. checking if
the read or write only accessed the first buffer. Hyper is one concrete
example of a library that has to do this dynamically:
https://github.com/hyperium/hyper/blob/0eaf304644a396895a4ce1f0146e596640bb666a/src/proto/h1/io.rs#L582-L594