Implement delayed acknowledgement

[Ralith]

Generating Acknowledgements:

In general, frequent feedback from a receiver improves loss and congestion
response, but this has to be balanced against excessive load generated by a
receiver that sends an ACK frame in response to every ack-eliciting packet. The
guidance offered below seeks to strike this balance.

An ACK frame SHOULD be generated for at least every second ack-eliciting packet.
This recommendation is in keeping with standard practice for TCP [RFC5681].

Detailed advice for cases when delaying acknowledgement is and is not appropriate follows. Due to the complexity cost, we should probably confirm the benefit of doing so with benchmarks.

A heavily-loaded Quinn receiver will already only generate one acknowledgement per batch of packets it consumes, so this may be less important for us than for a naive implementation.

[Ralith]

https://www.fastly.com/blog/measuring-quic-vs-tcp-computational-efficiency reports that the performance benefit of delayed ACKs is significant. We should pursue implementation.

[Ralith]

Also of potential interest is a draft extension for coordinating ACK delay configuration: https://tools.ietf.org/html/draft-iyengar-quic-delayed-ack-00

[Ralith]

We should also stop attaching outstanding the complete list of unacknowledged ACKs to every single outgoing packet, as this places excess load on the peer. Instead, we should only transmit an ACK if new packets have been received and either the maximum ACK delay has elapsed OR the number of new packets received exceeds a tunable threshold (default 2-10).

[Ralith]

An implementation should immediately send an ACK upon receiving an out of order packet.

[stormshield-damiend]

New draft that replace previous ones: https://datatracker.ietf.org/doc/draft-ietf-quic-ack-frequency/

[aochagavia]

I'll be working on this in the coming weeks

[aochagavia]

I have been digging into the code to figure out when ACKs are being sent. It looks like we are deviating from the spec in a few places, though. Could someone double-check my findings? (I might be mistaken, because of my unfamiliarity with the codebase.) I am also interested to hear why we are deviating from the spec, if that turns out to be the case.

As an introduction: we send most ACK frames in packets that contain other frames as well, instead of sending ACK-only packets. However, the spec indicates a bunch of scenarios in which we should send ACK frames right away, which means we should put them in an ACK-only packet if there is no other data to send. They are listed below:

1. 13.2.2-4: "A receiver SHOULD send an ACK frame after receiving at least two ack-eliciting packets". We seem to deviate from this suggestion, because we send an ACK every time an ack-eliciting packet is received (even if that means sending an ACK-only packet). This is the relevant function.
2. 13.2.1-9: "packets marked with the ECN Congestion Experienced (CE) codepoint in the IP header SHOULD be acknowledged immediately". We seem to ignore this suggestion. This is the relevant code.

Note that, if we want to fix 1, we need to implement the following parts of the spec (they are only necessary once you start delaying acks):

3. 13.2.1-7: "an endpoint SHOULD generate and send an ACK frame without delay when it receives an [out of order] ack-eliciting packet". We have no custom logic in place to detect out of order ack-eliciting packets, but it should be trivial to implement.
4. 13.2.1-1: "ack-eliciting packets MUST be acknowledged at least once within the maximum delay an endpoint communicated using the max_ack_delay transport parameter". We will need a timer to enforce this, which should also be easy enough.

[Ralith]

Your analysis looks correct to me. Our ACK transmit logic is just fairly primitive; we conform to the behavior required by the spec, but have not invested heavily in optimizing it historically. Those improvements sound good.