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: change AsyncRead to use a ReadBuf #2758

Merged
merged 11 commits into from Aug 14, 2020
Merged

io: change AsyncRead to use a ReadBuf #2758

merged 11 commits into from Aug 14, 2020

Conversation

seanmonstar
Copy link
Member

This changes AsyncRead::poll_read from being passed a &mut [u8] to be passed a &mut ReadBuf, which is a type that encapsulates the possibility of the read buffer containing uninitialized memory. For background, see #2716.

tokio/src/io/read_buf.rs Outdated Show resolved Hide resolved
tokio/src/io/read_buf.rs Outdated Show resolved Hide resolved
@seanmonstar
Copy link
Member Author

CI is green.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io labels Aug 12, 2020
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looks great! Nothing blocking, but would be nice to expand some of the safety comments even if they are a bit redundant.

tokio-util/src/compat.rs Show resolved Hide resolved
tokio/src/io/async_read.rs Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
tokio/src/io/util/take.rs Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Show resolved Hide resolved
tokio/src/net/tcp/stream.rs Show resolved Hide resolved
tokio/src/net/unix/stream.rs Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
tokio-test/src/io.rs Show resolved Hide resolved
tokio-util/src/compat.rs Show resolved Hide resolved
tokio/src/io/async_read.rs Outdated Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
tokio/src/io/read_buf.rs Outdated Show resolved Hide resolved
tokio/src/io/util/read_to_end.rs Outdated Show resolved Hide resolved
tokio/src/io/util/read_to_end.rs Outdated Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Looking good. Some smaller comments and questions inline.

tokio/src/io/read_buf.rs Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
let r = (*self).get_mut().read(buf);
// We can't assume the `Read` won't look at the read buffer,
// so we have to force initialization here.
let r = (*self).get_mut().read(buf.initialize_unfilled());
Copy link
Member

Choose a reason for hiding this comment

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

Types like TcpStream go via this path. We will want to avoid initializing memory in cases where it is not necessary. This can be done in a later PR, but we should track it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't believe neither TcpStream nor UnixStream hit this path. They both use their own poll_read_priv, which skips PollEvented.

pos: 0,
cap: 0,
}
let buffer = vec![0; capacity];
Copy link
Member

Choose a reason for hiding this comment

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

This switches to initializing the memory. Do we want to track this to fix later?

Copy link
Member Author

Choose a reason for hiding this comment

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

This switches to just using alloc_zeroed. I guessed that the cost is not likely noticeable, here in making a BufReader. If it is, we would need more complicated logic to keep a buffer: Box<[MaybeUninit<u8>]>, initialized: usize so we don't keep re-initializing it over and over.

let n = ready!(me.inner.poll_read(cx, &mut buf[..max]))?;
let max = std::cmp::min(buf.remaining() as u64, *me.limit_) as usize;
// Make a ReadBuf of the unfulled section up to max
let mut b = unsafe { ReadBuf::uninit(&mut buf.unfilled_mut()[..max]) };
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a ReadBuf::take(&mut self, n: usize) -> ReadBuf<'_> that does this logic? It seems useful.

tokio-test/src/io.rs Outdated Show resolved Hide resolved
tokio/src/io/read_buf.rs Show resolved Hide resolved
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks 👍. I think we should get this merged sooner than later to avoid merge conflicts. Could you track the remaining questions / tweaks in an issue and tag it w/ 0.3?

@carllerche
Copy link
Member

I opened #2769 to track the remaining items. Please check I didn't forget anything.

@carllerche carllerche merged commit c393236 into master Aug 14, 2020
@carllerche carllerche deleted the io-read-buf branch August 14, 2020 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants