Keep reading when there's space in the buffer for tokio::io::copy

[Darksonn]

This is the current poll method for tokio::io::copy and tokio::io::copy_bidirectional.

tokio/tokio/src/io/util/copy.rs

Lines 28 to 76 in adad8fc

	pub(super) fn poll_copy<R, W>(
	&mut self,
	cx: &mut Context<'_>,
	mut reader: Pin<&mut R>,
	mut writer: Pin<&mut W>,
	) -> Poll<io::Result<u64>>
	where
	R: AsyncRead + ?Sized,
	W: AsyncWrite + ?Sized,
	{
	loop {
	// If our buffer is empty, then we need to read some data to
	// continue.
	if self.pos == self.cap && !self.read_done {
	let me = &mut *self;
	let mut buf = ReadBuf::new(&mut me.buf);
	ready!(reader.as_mut().poll_read(cx, &mut buf))?;
	let n = buf.filled().len();
	if n == 0 {
	self.read_done = true;
	} else {
	self.pos = 0;
	self.cap = n;
	}
	}
	// If our buffer has some data, let's write it out!
	while self.pos < self.cap {
	let me = &mut *self;
	let i = ready!(writer.as_mut().poll_write(cx, &me.buf[me.pos..me.cap]))?;
	if i == 0 {
	return Poll::Ready(Err(io::Error::new(
	io::ErrorKind::WriteZero,
	"write zero byte into writer",
	)));
	} else {
	self.pos += i;
	self.amt += i as u64;
	}
	}
	// If we've written all the data and we've seen EOF, flush out the
	// data and finish the transfer.
	if self.pos == self.cap && self.read_done {
	ready!(writer.as_mut().poll_flush(cx))?;
	return Poll::Ready(Ok(self.amt));
	}
	}
	}

However, as long as self.cap < 2048, it would be possible to attempt to read more data into the buffer. We should do this. One might also consider copying data from the end of the buffer to the start to make space for further reading.

See also #3572.

[0xPoe]

I would like to try this.

[Darksonn]

Go ahead!

[0xPoe]

@cssivision Can't you see I'm trying to fix this?

Shouldn't you ask before you do it? Don't waste my time right?

It doesn't matter. Whatever.

[Darksonn]

Please be more careful when you take on PRs that someone have already claimed. If you are concerned that it is stalled, you should ask whether they are still working on it.

[0xPoe]

Yes, you are right.

@cssivision You don't have to close your PR, I'm just saying you should have asked me in advance instead of just submitting a PR, which would have caused everyone's time to be wasted. Please feel free to reopen your PR.(If you don't want to, that's fine) Thanks!

[rneswold]

Is this a real problem that needs to be handled? I find it hard to believe that, with modern drives and network speeds, the 2k buffer isn't filled all the time. I started a conversation that resulted in #3722 because I found reading and writing 2k at a time severely limited throughput.

I've found that reducing the calls into the kernel can increase performance due to context switches being a fairly expensive operation. Checking to see if a socket is readable or writable probably results in a kernel call (via ioctl) and then being added to the underlying poll is a call, albeit for all handles the scheduler blocks on. Making it do this multiple times for a 2k buffer seems like the wrong direction.

Unless you're targeting tiny machines (I don't know if Rust compiles code for embedded micros) 2k is a tiny amount of RAM (barely bigger than an MTU and much smaller than a GigE jumbo packet!)

I'm asking that, if this is a feature that is to be added, please benchmark it to see how it performs in reading or writing to a gigabit Ethernet link.

[rneswold]

we should not poll that resource again during this poll

That's good to know. However, I believe my other concerns are still valid. io::copy is a valuable building block but, if it doesn't transfer data quickly, everyone will be writing their own performant version.

[Darksonn]

Yeah, we should probably change the default buffer size too. The one in std uses 8k, so I guess that would be reasonable?

[rneswold]

8k should definitely be the minimum since Jumbo frames may be more common as GigE becomes the "slowest" network interface. It'll cut the number of kernel calls to a quarter of they are now, at least.

Another thing to consider is TCP's NO_DELAY option. Some network traffic is chit-chatty (e.g. ssh and telnet.) By default, TCP tries to buffer up more data to fill the MTU. But for login sessions or protocols with short request/reply messages, you don't want to wait for a full buffer to send because more data may be held off until a reply is received. So some applications turn on the NO_DELAY option for the socket so that small packets are sent immediately.

Trying to fill the buffer defeats that option. So I'd avoid trying to fill the 8k buffer; just send what was received. This covers short exchanges that don't fill the buffer and large data transfers that completely fill it.

(in other words, short messages won't see a benefit of 8k but large data transfers will.)

[Darksonn]

When it comes to NO_DELAY, that's not something io::copy controls. You can set it on the socket when you create it.

As for filling the buffer, the design I had in mind would only try to read more data into the buffer if the AsyncWrite is not able to send all of the data immediately. So it seems to me that this would not affect the case where the IO resources are extremely fast. It's still worth testing it of course.