fix(http3): always qlog on send_buffer() #1876
Conversation
`neqo_http3::SendMessage` calls `qlog::h3_data_moved_down()` whenever it moves data down to the QUIC layer. `SendMessage` moves data down to the QUIC layer either directly via `self.stream.send_atomic` or indirectly buffered through `self.stream.send_buffer`. Previously only one of the 3 calls to `self.stream.send_buffer` would thereafter call `qlog::h3_data_moved_down()`. This commit moves the `h3_data_moved_down` call into `self.stream.send_buffer`, thus ensuring the function is always called when data is moved. In addition, `self.stream.send_atomic` now as well does the qlog call, thus containing all qlog logic in `buffered_send_stream.rs` instead of `send_message.rs`.
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
Benchmark resultsPerformance differences relative to ed19eb2.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
|
Performance regression? |
That is surprising, given that qlog is disabled on both client and server. Line 74 in ed19eb2 Will investigate. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1876 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 110 110
Lines 35810 35811 +1
=======================================
+ Hits 33390 33391 +1
Misses 2420 2420 ☔ View full report in Codecov by Sentry. |
Consecutive run, without any changes, shows no regressions. |
|
d15e3e0 replaces the nested |
|
I don't think the performance regressions seen in the first run are related. @larseggert mind giving this pull request a review? |
neqo_http3::SendMessagecallsqlog::h3_data_moved_down()whenever it moves data down to the QUIC layer.SendMessagemoves data down to the QUIC layer either directly viaself.stream.send_atomicor indirectly buffered throughself.stream.send_buffer.Previously only one of the 3 calls to
self.stream.send_bufferwould thereafter callqlog::h3_data_moved_down().Calls missing the follow-up qlog call:
neqo/neqo-http3/src/send_message.rs
Line 173 in ed19eb2
neqo/neqo-http3/src/send_message.rs
Line 303 in ed19eb2
This commit moves the
h3_data_moved_downcall intoself.stream.send_buffer, thus ensuring the function is always called when data is moved. In addition,self.stream.send_atomicnow as well does the qlog call, thus containing all qlog logic inbuffered_send_stream.rsinstead ofsend_message.rs.