Previously, we limited the number of streams that could be opened by
the peer but not `accept`ed by the application. However, to guarantee
bounded resource use, applications will typically want to limit the
number of streams they process concurrently. While this could
be *approximately* implemented at the application layer by controlling
calls to accept, that approach has a significant drawback: If streams
are slow to process, the worst-case per-stream latency observed by a
peer who opens the maximum number of streams can be arbitrarily bad,
because streams may be opened above the limit the local application is
willing to process. Reducing the number of unaccepted streams
tolerated can reduce the proportion of streams affected, but reducing
it too low will increase the number of round trips required to open
any given number of streams, increasing average latency significantly.
As a side benefit, this reduces the effort needed for applications to
limit concurrency to a fixed quantity, which is expected to be the
overwhelmingly common case. Should a use case for dynamic concurrency
limits arise, we can expose a setter.
The pacer's behavior currently makes the library extremely inefficient.
Each `Pacer::delay` call would only allow a single
datagram to be sent, and then instruct the connection to wait a tiny
time-slice (which is far smaller than the timer granularity). When the timer
really elapses (e.g. a tokio timer after 1ms) only 1 packet can be sent
again.
This change improves on this by deriving the pacer capacity based on
how big bursts should be, and how much delay we want to have between
those. A pacer delay bigger than timer granularity is desirable for
efficiency and performance reasons. Here 2ms had been chosen, which
had proven effective in tests with an injected RTT.
The pacing implementation did so far not do anything. The reason for
that is the timestamp when tokens had been last generated was never
updated. Therefore each Pacer::delay call where not enough tokens
had been available calculated tokens based on the time back to when
the Pacer was initially created, which fully refills the capacity.
This is easily fixed by storing the timestamp when tokens are replenished.
The variable `defragmented` wasn't properly updated in the
`read_chunks` method, since it directly manipulated the map instead
of delegating to the `Assembler::pop` method. This caused an underflow
when trying to insert more data into the map.
This change fixes that, and adds a repro test for it.
Fixes#982
During testing with a multithreaded runtime I discovered that threads
block a fair amount of time on Mutexes, even though everything in the
state machine should be non-blocking.
By inserting some more timing checks I discovered that the `poll_write`
call took more than 10ms and locked the Mutex for this duration. This
happened due to the `BytesMut::extend_from_slice` taking this time.
The reason here is that extending the buffer require a reallocation of
the buffer and a copy of the complete data. This can be a big size
(up the maximum internal buffer size) - even if only a tiny chunk of
data is added.
This change improves on this by using a non-contiguous send buffer.
New data is appended as a new `Bytes` segment, which only requires
an allocation for this particular size. The 10ms blocking problem is
gone with this.
Another benefit of this change is that it can easily enable zero-copy
writes by accepting `Bytes` as a parameter in the user-facing API.
This is however not performed here yet.
```
Jan 18 11:14:00.064 WARN quinn_proto::connection::send_buffer: self.unacked.extend_from_slice(data[..1048576]) took 10.3452ms. Now unacked size: 94924462
Jan 18 11:14:00.064 WARN quinn_proto::connection::streams: Long Send::write. Total: 10.3943ms, Budget check: 0ns, data_len: 1048576, written: 1048576
Jan 18 11:14:00.064 WARN quinn_proto::connection::streams: Long Streams::write. Total: 10.4203ms, Limit check: 100ns, Unsent check: 200ns, Write: 10.4202ms. data_len: 1048576, written: 1048576
Jan 18 11:14:00.064 WARN quinn::streams: Long poll_write time: close_check 100ns, write_time 10.4456ms, end_time 10.4456ms
```
If an a range is acknowledged where a part of it had been previously
acknowledged - ignore the acknowledged range. While the
retransmission logic guarantees this property at the moment, it
might not always hold true.
The sending data on the benchmark failed it panicked due
to an unwrap and thereby showed no result. This propagates
the first client-side error instead of panicking - which will also
preserve the statistics.
Dirkjan Ochtman