fix(SendMessage): use SendStream::set_writable_event_low_watermark #1838
Conversation
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.
Benchmark resultsPerformance differences relative to ed19eb2.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1838 +/- ##
==========================================
- Coverage 93.27% 93.26% -0.02%
==========================================
Files 110 110
Lines 35793 35864 +71
==========================================
+ Hits 33387 33448 +61
- Misses 2406 2416 +10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
LGTM but I would like @KershawChang or @martinthomson to also take a look.
Should we see a performance increase in the benchmarks?
There was a problem hiding this comment.
Overall looks good, but it'd be good to see @martinthomson 's comment on this.
Which of his comments? (I can't seem to find one on this PR.) |
Sorry for any confusion. I assumed that Martin would add his review comments to this change. I'd like him to have a look as well. |
Intuitively yes, especially since it fixes #1821 along the way, i.e. Running the That said, as you say, the benchmarks show a regression:
I don't have a good understanding why yet. Also I don't know how representative this benchmark currently is with |
Let's see whether the "Download" benchmark is consistent.
Consecutive run no longer shows a regression. Not a performance proof for this patch, but rather another datapoint that the benchmark is unreliable. |
|
Holding off until @martinthomson has reviewed. |
|
All review comments addressed. I still need to look into the current test failures. |
|
Ping me when you sort out the test failures. The code looks fine, but you might have changed test conditions. |
|
@martinthomson test failure was due to a simple double borrow attempt. See 8d66880. Ready to merge now. |
|
@martinthomson in case your intention was to merge here: I think you will have to resolve your previous request for change. |
Previously
SendMessage::send_datacould 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_watermarkwhich is then used inSendMessage::send_datato signal toSendStreamthe minimum required send space (low watermark) for the next send. Once reached,SendStreamemits aSendStreamWritableevent, eventually triggering anotherSendMessage::send_data.Alternative to #1835. Compared to #1835, this fix does not utilize the
SendMessagebuffer, 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 inneqo-transportandneqo-http3.Fixes #1819.
Secondarily, this fixes #1821 as well.
Thank you Martin and Lars for the input on #1819 out-of-band.