Performance reading from serial ports suffers due to excessive read() calls

[rkuris]

Version

$ cargo tree | grep tokio | sed -e 's/.*\(tokio.*v[^ ]*\).*/\1/' | sort | uniq
tokio-io-timeout v1.2.0
tokio-macros v1.8.2
tokio-serial v5.4.4
tokio-stream v0.1.11
tokio-util v0.7.4
tokio v1.23.0

Platform

$ uname -a
Linux rk-dell 5.15.0-56-generic #62-Ubuntu SMP Tue Nov 22 19:54:14 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Description
When reading from a serial port using AsyncFd in O_NONBLOCK mode, an extra read() call is performed:

read(16, "1", 3013)                     = 1
read(16, 0x55cc4763f34c, 3012)          = -1 EAGAIN (Resource temporarily unavailable)
epoll_wait(3, [{events=EPOLLIN|EPOLLOUT, data={u32=2, u64=2}}], 1024, -1) = 1

One can assume that if the buffer didn't fill up, the following read won't succeed, so it should proceed directly to epoll_wait instead of attempting the read and having it fail. Note that on some serial devices, they will never return more than a smaller buffer (say 64) so I would encourage this optimization to only take place when a single character is read.

This was fixed for non-AsyncFd reads here:
tokio-rs PR-4970
tokio-rs PR-4840

[Noah-Kennedy]

I said this in discord, but this cannot be fixed for our current AsyncFd API without a significant breaking change which would not be backwards compatible, and may well be erroneous.

This optimization relies on the fact that a partial read or write to a socket in a *nix OS which is a continuous byte stream (e.g TCP but not UDP) implies that the descriptor no longer has readiness for the IO direction in question as it could not fill the buffer.

Unfortunately, we don't know if the file descriptor we are wrapping is a continuous stream. It could very well be something like a seqpacket or datagram socket, in which case partial reads and writes are to be expected in cases where there is still data in the buffer to be read. Furthermore, without specialization, I'm not sure that this is even possible given that our APIs are generic over the return type (it need only be io::Result<T> for a type T).

I think the only way to do this would maybe be to add a public type which acts like the internal PollEvented type which our TcpStream and UnixStream types are built on, and provides an AsyncRead and an AsyncWrite implementation.

[Darksonn]

Hmm, lets imagine that we're writing a wrapper around AsyncFd for some type where we know that a partial read implies that the socket is no longer ready. Then, we could perhaps implement reading in the following manner:

1. Wait for readiness using poll_read_ready.
2. Perform the read operation using libc.
3. If the read fails with WouldBlock, or if we get a partial read, then call clear_ready. Otherwise call retain_ready.

Would this work?

(I understand that this would need to happen in the external library, rather than in Tokio.)

[Noah-Kennedy]

That would work.

[Noah-Kennedy]

My suggestion of another type was for if we want to add something which does this to make things easier on users.

This feels like something that could potentially work fine in tokio-util.

[Darksonn]

Perhaps we could add that. I think we should also ensure to document how to properly implement the optimization in the documentation for AsyncFd.

[Noah-Kennedy]

I can go ahead and add this then, both to tokio as docs and to tokio-util as a wrapper type.

[Noah-Kennedy]

Working on this now. There are two approaches we can take.

The first approach would be to do this in tokio itself and put it in the io::unix module like with AsyncFd. This would be pretty easy to do, as we would just be adding a thin wrapper around PollEvented. The second approach would be to do this in tokio-util as a wrapper around AsyncFd and to basically duplicate much of PollEvented.

@Darksonn what are your thoughts here? I personally really don't have much of a preference between the two options, although I do slightly lean towards tokio-util because we can add a few different types (including one that isn't AsyncRead/Write for non-streaming IO) and not be bound by the tokio stability guarantees.