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.

Sign up for GitHub

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

io: reduce syscalls in poll_write #4970

Merged
merged 1 commit into from
Sep 2, 2022
+20 −1
Merged

io: reduce syscalls in poll_write #4970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion tokio/src/io/poll_evented.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,26 @@ feature! {
&'a E: io::Write + 'a,
{
use std::io::Write;
self.registration.poll_write_io(cx, || self.io.as_ref().unwrap().write(buf))

loop {
let evt = ready!(self.registration.poll_write_ready(cx))?;

match self.io.as_ref().unwrap().write(buf) {
Ok(n) => {
// if we write only part of our buffer, this is sufficient on unix to show
// that the socket buffer is full
if n > 0 && (!cfg!(windows) && n < buf.len()) {
self.registration.clear_readiness(evt);
}
Comment on lines +197 to +201
Copy link
Member

Choose a reason for hiding this comment

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

this is admittedly very nitpicky, but would it make sense to write this instead:

Suggested change
// if we write only part of our buffer, this is sufficient on unix to show
// that the socket buffer is full
if n > 0 && (!cfg!(windows) && n < buf.len()) {
self.registration.clear_readiness(evt);
}
// if we write only part of our buffer, this is sufficient on unix to show
// that the socket buffer is full
#[cfg(not(windows))]
{
if n > 0 && n < buf.len() {
self.registration.clear_readiness(evt);
}
}

so that we skip the n > 0 check on windows, where the if condition will always evaluate false anyway? or is the compiler smart enough to const-fold that too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't particularly care either way, but I would be very surprised if LLVM wasn't able to optimize this away, considering that we are inserting a const boolean value here that guarantees the outcome of the whole expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going leave this as-is, since we are already doing the same thing for reads, and I'd like to keep this change as just "copy over the behavior for reads". I'm perfectly happy with another PR that changes both in the manner you suggested though.

I might also be slightly influenced by not wanting to rerun CI here as well.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, i'm definitely fine with this as-is!


return Poll::Ready(Ok(n));
},
Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
self.registration.clear_readiness(evt);
}
Err(e) => return Poll::Ready(Err(e)),
}
}
}

#[cfg(feature = "net")]
Expand Down