fix(SendMessage): always use available send space #1835
Conversation
Always use up send space on QUIC layer to ensure receiving `ConnectionEvent::SendStreamWritable` event when more send space is available. See mozilla#1819 for details. This commit implements option 2. Fixes mozilla#1819.
| if buf.is_empty() { | ||
| qerror!([self], "zero-length send_data"); | ||
| return Err(Error::InvalidInput); | ||
| } |
There was a problem hiding this comment.
Same as in neqo_transport::SendStream::send_internal which is called by this function anyways. Thus just moving the return Err up the stack, which allows the else if available <= 2 to depend on buf not to be empty.
neqo/neqo-transport/src/send_stream.rs
Lines 1253 to 1256 in 3e53709
Benchmark resultsPerformance differences relative to 3e53709.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
| // Rare case, where available send space at most fits the data frame | ||
| // header (min 2 bytes), but no data. Don't return `Ok(0)`, but | ||
| // instead use up `available` anyways, with a small but non-zero | ||
| // data frame, buffering the remainder, thereby signaling to the | ||
| // QUIC layer that more is needed, and thus ensure to receive | ||
| // [`neqo_transport::ConnectionEvent::SendStreamWritable`] when more | ||
| // send space is available. | ||
| const MIN_DATA: usize = 1; // arbitrary small value, larger zero | ||
| return self | ||
| .send_data_atomic(conn, &buf[..MIN_DATA]) | ||
| .map(|()| MIN_DATA); |
There was a problem hiding this comment.
I'd like to see a test showing that this is OK. I'm fairly sure that it isn't.
You aren't sending a DATA frame header for this. That header is always at least 2 bytes.
There was a problem hiding this comment.
@martinthomson the DATA frame header is sent as part of the call to self.send_data_atomic:
neqo/neqo-http3/src/send_message.rs
Lines 295 to 305 in d74ad5e
Not saying that this doesn't need a test.
There was a problem hiding this comment.
Ah, so that will result in us sending three bytes to the stream, in a case where we only have space for 2 or less. How does that ever work?
There was a problem hiding this comment.
in a case where we only have space for 2 or less
We will use up the available space and buffer the remaining. E.g. say that we have 2 bytes available, we send the header (2bytes) and buffer 1 byte.
Once more space is available, we get an event (because we previously exhausted the send space) and can thus first empty the 1byte from the buffer and then send more goodput.
Does that make sense @martinthomson?
(Not particularly happy with this hack. Still thinking about alternatives.)
Edit: alternative #1838
Previously `SendMessage::send_data` could stall, if less than the minimum message size is available to be sent. See mozilla#1819 for details. This commit implements solution (3) proposed in mozilla#1819. This commit introduces `SendStream::set_writable_event_low_watermark` which is then used in `SendMessage::send_data` to signal to `SendStream` the minimum required send space (low watermark) for the next send. Once reached, `SendStream` emits a `SendStreamWritable` eventually triggering another `SendMessage::send_data`. Alternative to mozilla#1835. Compared to mozilla#1835, this fix does not utilize the `SendMessage` buffer, thus does not introduce an indirection to the send path. In addition, under the assumption that available send space is increased in larger batches, this fix does not send tiny data frames (2 byte header, 1 byte goodput). Downside, compared to mozilla#1835, is that it requires both changes in `neqo-transport` and `neqo-http3`. Secondarily, this fixes mozilla#1821 as well.
Previously `SendMessage::send_data` could stall, if less than the minimum message size is available to be sent. See mozilla#1819 for details. This commit implements solution (3) proposed in mozilla#1819. This commit introduces `SendStream::set_writable_event_low_watermark` which is then used in `SendMessage::send_data` to signal to `SendStream` the minimum required send space (low watermark) for the next send. Once reached, `SendStream` emits a `SendStreamWritable` eventually triggering another `SendMessage::send_data`. Alternative to mozilla#1835. Compared to mozilla#1835, this fix does not utilize the `SendMessage` buffer, thus does not introduce an indirection to the send path. In addition, under the assumption that available send space is increased in larger batches, this fix does not send tiny data frames (2 byte header, 1 byte goodput). Downside, compared to mozilla#1835, is that it requires both changes in `neqo-transport` and `neqo-http3`. Secondarily, this fixes mozilla#1821 as well.
|
Closing here in favor of #1838. |
…1838) * fix(SendMessage): use SendStream::set_writable_event_low_watermark Previously `SendMessage::send_data` could stall, if less than the minimum message size is available to be sent. See #1819 for details. This commit implements solution (3) proposed in #1819. This commit introduces `SendStream::set_writable_event_low_watermark` which is then used in `SendMessage::send_data` to signal to `SendStream` the minimum required send space (low watermark) for the next send. Once reached, `SendStream` emits a `SendStreamWritable` eventually triggering another `SendMessage::send_data`. Alternative to #1835. Compared to #1835, this fix does not utilize the `SendMessage` buffer, thus does not introduce an indirection to the send path. In addition, under the assumption that available send space is increased in larger batches, this fix does not send tiny data frames (2 byte header, 1 byte goodput). Downside, compared to #1835, is that it requires both changes in `neqo-transport` and `neqo-http3`. Secondarily, this fixes #1821 as well. * Move const * Add documentation * Add SendStream test * Fix intra doc links * Add neqo-http3 test * Replace goodput with payload * Re-trigger benchmarks Let's see whether the "Download" benchmark is consistent. * Rename emit_writable_event to maybe_emit_writable_event * Replace expect with unwrap * Use NonZeroUsize::get * Replace expect with unwrap * %s/Actually sending/Sending * Typo * Have update() return available amount * Document setting once would suffice * Reduce verbosity * fix: drop RefCell mutable borrow early
Always use up send space on QUIC layer to ensure receiving
ConnectionEvent::SendStreamWritableevent when more send space is available.See #1819 for details. This commit implements option 2.
Fixes #1819.
Not particularly happy with the fix. Though, as of today, I am considering this to be the least intrusive option. Will ponder a bit more before marking this as ready-for-review. Input welcome anyways.