-
Notifications
You must be signed in to change notification settings - Fork 132
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 support for zerocopy write to the TCP socket #237
base: master
Are you sure you want to change the base?
Conversation
hm,
and
|
/// ```no_run | ||
/// use std::net::SocketAddr; | ||
/// use tokio_uring::net::TcpListener; | ||
/// ``` |
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 let you figure out why your test and the CI test are different. But this seems to hijack a documentation example for write_all
with one for write_zc
. That doesn't seem to fly.
/// > at writes over around 10 KB. | ||
/// | ||
/// Note: Using fixed buffers [#54](https://github.com/tokio-rs/tokio-uring/pull/54), avoids the page-pinning overhead | ||
pub async fn write_zc<T: BoundedBuf>(&self, buf: T) -> crate::BufResult<usize, T> { |
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.
What's the purpose of calling this write_zc
? Where does that come from? Does the std or tokio libraries have a name like that for a TCP connection? If not, at first glance it seems better to stick with the names of either the io_uring interface or maybe the io-uring crate API.
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.
write_zc
is zero copy version of existing write
, similar to how UdpSocket::send_zc
is zero copy version of UdpSocket::send
.
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.
Tokio doesn't have this, but Linux sockets do, and it's a very useful optimisation. Offloading data copy from your CPU to your NIC can vastly reduce CPU overhead
I believe it unusual not to provide a high level description of what is going on with the PR. That's what the top description is for. A reviewer, or anyone else, should not have to read through the code diffs to figure out the intent and the way the intent is being handled. If you read through the git log for this repo, you will see the level of detail that is usually given. And unless you are absolutely sure of yourself. It is safer to create an Issue first talking about what you think is missing and what you would propose doing about it. |
And if you do create a PR but you didn't think it was ready for review and submission, please make it a draft. |
Intorduce zero copy version of
TcpStream::write
usingwrite_zc
name.It behaves almost the same, except returned future will be complete when remotes side ACKs receipt of the buffer content, unlike
TcpStream::write
which completes when data copied into local TCP socket kernel buffers.