Simplify UpdateRtt pseudocode #2610
Conversation
janaiyengar
added
editorial
| adjusted_rtt = latest_rtt | ||
| if (latest_rtt - min_rtt > ack_delay): | ||
| if (latest_rtt > min_rtt + ack_delay): | ||
| adjusted_rtt = latest_rtt - ack_delay |
There was a problem hiding this comment.
You can simply do latest_rtt -= ack_delay and then use latest_rtt instead of adjusted_rtt below, since latest_rtt is not used anywhere anymore
There was a problem hiding this comment.
(But if you want to use the global here, maybe that adjustment to the global is wrong?)
There was a problem hiding this comment.
The prose relies on latest_rtt in descriptions. Adjusting that value directly would confuse things.
| @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space): | |||
|
|
|||
|
|
|||
| UpdateRtt(latest_rtt, ack_delay): | |||
There was a problem hiding this comment.
Why is latest_rtt passed in as a parameter? It's already a global. Does this parameter differ from the global?
There was a problem hiding this comment.
latest_rtt probably shouldn't be global.
There was a problem hiding this comment.
It's used in DetectLostPackets, so it needs to be global.
To avoid confusion with scoping rules, the function parameter should have a different name than the global variable though.
| @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space): | |||
|
|
|||
|
|
|||
| UpdateRtt(latest_rtt, ack_delay): | |||
| if (smoothed_rtt == 0): | |||
| // First RTT sample. | |||
There was a problem hiding this comment.
Shouldn't you adjust by ack_delay here?
There was a problem hiding this comment.
I don't think so. #2607 covers this though.
| @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space): | |||
|
|
|||
|
|
|||
| UpdateRtt(latest_rtt, ack_delay): | |||
There was a problem hiding this comment.
latest_rtt probably shouldn't be global.
| @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space): | |||
|
|
|||
|
|
|||
| UpdateRtt(latest_rtt, ack_delay): | |||
| if (smoothed_rtt == 0): | |||
| // First RTT sample. | |||
There was a problem hiding this comment.
I don't think so. #2607 covers this though.
| adjusted_rtt = latest_rtt | ||
| if (latest_rtt - min_rtt > ack_delay): | ||
| if (latest_rtt > min_rtt + ack_delay): | ||
| adjusted_rtt = latest_rtt - ack_delay |
There was a problem hiding this comment.
The prose relies on latest_rtt in descriptions. Adjusting that value directly would confuse things.
No description provided.