Clarify OnAckReceived pseudocode #2140
Conversation
Ralith
force-pushed
the
ack-cleanup
branch
3 times, most recently
from
9385133
to
f9ad5a9
Compare
|
Split the |
There was a problem hiding this comment.
A minor nit, but this is a nice cleanup. Thanks! I'll approve, but @ianswett should take a look as well.
| @@ -716,6 +717,9 @@ Pseudocode for OnAckReceived and UpdateRtt follow: | |||
|
|
|||
| ~~~ | |||
| OnAckReceived(ack): | |||
| largest_acked_packet = max(largest_acked_packet, | |||
There was a problem hiding this comment.
I think this accurately describes the text we have, but I think we should add some text that you MAY use the largest acked from the most recent ACK, which could reduce spurious retransmits when one packet arrives far out of order?
There was a problem hiding this comment.
To clarify, I'm ok doing this in a follow up PR.
There was a problem hiding this comment.
you MAY use the largest acked from the most recent ACK, which could reduce spurious retransmits when one packet arrives far out of order
Any reason not to make this the sole recommendation? I had some trouble coming up with a case where that might occur, which is why I didn't take that route originally, but I think I see it now: when a reordered ACK newly acknowledges a packet such that the newly acknowledged packet is > ack.largest_acked - kPacketThreshold, but <= largest_acked_packet - kPacketThreshold.
There was a problem hiding this comment.
Er, wait, that's nonsense. Still having trouble seeing the case.
There was a problem hiding this comment.
It's the case when one packet is far out of order, and then subsequent packets arrive in order. ie: 100 is received, then 1, 2, 3, 4, 5, etc. I don't fully understand the cause, but I've seen small UDP packets jump ahead of large ones in a number of traces.
But I suspect Jana and I will merge this as is and we can add follow-up text about that issue if we think it's valuable.
martinthomson
added
editorial
I ran into a few points of confusion in the existing code:
largest_acked_packetis not explicitProcessECNreferences elements ofsent_packetsafter they are removed byOnPacketAckedDetectLostPacketsis passed an undefined variableThe fixes are trivial, except for the last: it isn't clear to me whether
DetectLostPacketswas intended to be called withack.largest_ackedorlargest_acked_packet. These only differ when ACKs are reordered, and because the prior invocation ofDetectLostPacketswith a higherlargest_ackedwould have already deemed lost all packets that the later invocation might deem lost, I don't think the behavior differs even then. Hence, removing the argument and consistently referencinglargest_acked_packetseems to be the simpler option.