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.

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
Merged

Conversation

Noah-Kennedy
Copy link
Contributor

This applies the same optimization made in #4840 to writes.

This applies the same optimization made in #4840 to writes.
Comment on lines +197 to +201
// 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);
}
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants