bneradt commented on code in PR #13554:
URL: https://github.com/apache/trafficserver/pull/13554#discussion_r3799295218
##########
src/iocore/net/QUICNetVConnection.cc:
##########
@@ -688,17 +688,22 @@ void
QUICNetVConnection::_handle_write_ready()
{
if (quiche_conn_is_established(this->_quiche_con)) {
+ const size_t budget =
QUICStream::compute_fair_send_budget(this->_last_writable_stream_count);
Review Comment:
**[P2] Avoid using the maximum budget for every stream on the first event.**
`_last_writable_stream_count` starts at 1, so an event whose current
iterator contains many streams gives all of them the 256 KB maximum. With the
configured 100-stream limit, that can queue or process roughly 25.6 MB in one
event instead of the previous 1.6 MB, delaying unrelated connections on the
same event thread. The QMux path has the same pattern. Please initialize
conservatively, enforce a connection-wide per-event cap, or otherwise account
for current contention.
##########
src/iocore/net/quic/QUICStream.cc:
##########
@@ -144,24 +155,28 @@ QUICStream::receive_data(QUICStreamIO &stream_io)
}
int64_t
-QUICStream::send_data(QUICStreamIO &stream_io)
+QUICStream::send_data(QUICStreamIO &stream_io, size_t max_bytes_this_event)
{
bool fin = false;
ssize_t len = 0;
[[maybe_unused]] ErrorCode error_code{0};
size_t written_this_event = 0;
+ // _write_vio.nbytes is set once when the VIO is armed and doesn't change
over the
+ // course of this call, so query it once instead of re-locking the adapter's
mutex
+ // for the same value on every loop iteration below.
+ const uint64_t total_len = this->_adapter->total_len();
- while (written_this_event < MAX_STREAM_SEND_BYTES_PER_EVENT) {
+ while (written_this_event < max_bytes_this_event) {
Review Comment:
**[P2] Cap a carried pending block to this event’s remaining budget.**
`_pending_send_block` can have been created under a previous 256 KB budget
and survive a partial write. If contention then reduces `max_bytes_this_event`
to 16 KB, the later `write_stream()` call still passes the entire remaining
block, allowing the stream to exceed the computed budget. Please cap the
submitted length to `max_bytes_this_event - written_this_event` (and only set
`fin` when the whole pending block is included). A unit test with a partial
first write followed by a smaller budget would cover this transition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
