perf(transport): remove Server::timers #1784
Conversation
mxinden
force-pushed
the
no-timer
branch
3 times, most recently
from
6c768e7
to
cb62ff3
Compare
|
I'd be OK to rip this out and limit our demo server to only being useful with a small number of active connections. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
- Coverage 93.05% 93.05% -0.01%
==========================================
Files 117 116 -1
Lines 36368 36101 -267
==========================================
- Hits 33843 33592 -251
+ Misses 2525 2509 -16 ☔ View full report in Codecov by Sentry. |
Benchmark resultsPerformance differences relative to 0751429.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
The current `neqo_transport::server::Server::timers` has a large performance overhead, especially when serving small amount of connections. See mozilla#1780 for details. This commit optimizes for the small-number-of-connections case, keeping a single callback timestamp only, iterating each connection when there is no other work to be done.
|
The |
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
|
Thank you for the review @martinthomson. Can you take another look? |
This reverts commit 61fcd28.
…lla#1800) This reverts commit 342e4e7. With mozilla#1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now reapply the patch removing `Server::timers`. More specifically, the actual bug fix on mozilla-central side: ``` rust let output = if self.response_to_send.is_empty() { output } else { // In case there are pending responses to send, make sure a reasonable // callback is returned. const MIN_INTERVAL: Duration = Duration::from_millis(100); match output { Output::None => Output::Callback(MIN_INTERVAL), o @ Output::Datagram(_) => o, Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)), } }; ``` See https://phabricator.services.mozilla.com/D209574.
This reverts commit 342e4e7. With #1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now reapply the patch removing `Server::timers`. More specifically, the actual bug fix on mozilla-central side: ``` rust let output = if self.response_to_send.is_empty() { output } else { // In case there are pending responses to send, make sure a reasonable // callback is returned. const MIN_INTERVAL: Duration = Duration::from_millis(100); match output { Output::None => Output::Callback(MIN_INTERVAL), o @ Output::Datagram(_) => o, Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)), } }; ``` See https://phabricator.services.mozilla.com/D209574.
The current
neqo_transport::server::Server::timershas a large performance overhead, especially when serving small amount of connections. See #1780 for details.This commit optimizes for the small-number-of-connections case, keeping a single callback timestamp only, iterating each connection when there is no other work to be done.
Draft for now.
I need more context to know what to optimize for.
See #1780 (comment).
Opening up early to get some performance stats from the CI benchmark machine.