fix(transport): only emit SendStreamWritable on ack if prev. blocked #1821
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`neqo_transport::SendStream` emits `ConnectionEvent::SendStreamWritable` to inform a user that they can write more data into a stream. It emits this event in 3 cases: - on `SendStream::new` - on `SendStream::set_max_data` - on `SendStream::mark_as_acked` Emitting a `SendStreamWritable` event, even though writing wasn't previously blocked, is useless. That is why `SendStream::set_max_data` only emits the event if sending was previously blocked on stream flow control. https://github.com/mozilla/neqo/blob/c004359a817ffdea33394e94944d2f882e7e78af/neqo-transport/src/send_stream.rs#L1206-L1216 This commit adds the corresponding check to `SendStream::mark_as_acked` as well. I.e. it checks whether sending was previously blocked on the tx buffer size, before emitting a `SendStreamWritable` event. This is especially relevant for `SendStream::mark_as_acked`, as each received ack would otherwise result in a `SendStreamWritable` event. The event in itself isn't expensive. The logic it triggers is.
mxinden
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
=======================================
Coverage 93.13% 93.13%
=======================================
Files 117 117
Lines 36353 36383 +30
=======================================
+ Hits 33857 33887 +30
Misses 2496 2496 ☔ View full report in Codecov by Sentry. |
|
I expect the latest benchmark stall to be due to #1819. |
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
Apr 17, 2024
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.
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
Apr 17, 2024
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.
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 7, 2024
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
neqo_transport::SendStreamemitsConnectionEvent::SendStreamWritableto inform a user that they can write more data into a stream.It emits this event in 3 cases:
SendStream::newSendStream::set_max_dataSendStream::mark_as_ackedEmitting a
SendStreamWritableevent, even though writing wasn't previously blocked, is useless. That is whySendStream::set_max_dataonly emits the event if sending was previously blocked on stream flow control.neqo/neqo-transport/src/send_stream.rs
Lines 1206 to 1216 in c004359
This commit adds the corresponding check to
SendStream::mark_as_ackedas well. I.e. it checks whether sending was previously blocked on the tx buffer size, before emitting aSendStreamWritableevent.This is especially relevant for
SendStream::mark_as_acked, as each received ack would otherwise result in aSendStreamWritableevent. The event in itself isn't expensive. The logic it triggers is.As an example, I was seeing > 200
SendStreamWritableevents queued at once in theDownloadbenchmark.I suggest prioritizing the related #1819 (bug) before this pull request (optimization).