Replaces uses of TLP and RTO with PTO #134
Conversation
| @@ -1274,6 +1274,7 @@ int quicly_encode_transport_parameter_list(ptls_buffer_t *buf, int is_client, co | |||
| { pushv(buf, params->max_streams_uni); }); | |||
| } | |||
| PUSH_TRANSPORT_PARAMETER(buf, QUICLY_TRANSPORT_PARAMETER_ID_ACK_DELAY_EXPONENT, { pushv(buf, QUICLY_ACK_DELAY_EXPONENT); }); | |||
| PUSH_TRANSPORT_PARAMETER(buf, QUICLY_TRANSPORT_PARAMETER_ID_MAX_ACK_DELAY, { pushv(buf, QUICLY_LOCAL_MAX_ACK_DELAY); }); | |||
There was a problem hiding this comment.
Let's surround this with if (QUICLY_DEFAULT_MAX_ACK_DELAY != QUICLY_LOCAL_MAX_ACK_DELAY). Every byte that's being used under 3x rule is precious.
| ++r->rto_count; | ||
| /* PTO */ | ||
| if (r->pto_count == 0) | ||
| ++r->pto_count; | ||
| *num_packets_to_send = 2; |
There was a problem hiding this comment.
Do you think it's reasonable to keep the API that returns the maximum number of packets that should be sent? I ask this because I think there's a potential of changing this to a boolean value indicating if any packet should be sent.
There was a problem hiding this comment.
Yeah, I was thinking about that, but it fell off my radar. I can do that.
lib/quicly.c
Outdated
| if (bytes_acked > 0) { | ||
| conn->egress.loss.pto_count = 0; |
There was a problem hiding this comment.
This change removes the abstraction that was provided by quicly_loss_on_packet_acked. I do not have a strong preference on eliminating the abstraction (the reason I created the abstraction is because I am unfamiliar with loss recovery and therefore wanted a clean-cut API as much as possible), but wonder what your opinion is on how much split loss recovery and the transport logic should be.
There was a problem hiding this comment.
That is a good point. I wanted to clean up the abstraction a bit, but I'll add the abstraction back in now and do the cleanup later.
This PR does the first pass of replacing TLP and RTO with PTO. Persistent congestion is not yet detected, and I've filed #132 for doing this.
With TLP and RTO, the sender would prioritize sending new data on TLP and old data on RTO. This change prioritizes new data over old on a PTO. We may want to revisit this in the scheduler, filed #133 for this.
This PR also fixes a bug with max_ack_delay in transport params.
Fixes #108.