Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

!editorial: simplify pseudo code (recovery) #3384

Closed
wants to merge 1 commit into from
Closed

!editorial: simplify pseudo code (recovery) #3384

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 1 addition & 3 deletions draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -346,9 +346,7 @@ On subsequent RTT samples, smoothed_rtt and rttvar evolve as follows:

~~~
ack_delay = min(Ack Delay in ACK Frame, max_ack_delay)
adjusted_rtt = latest_rtt
if (min_rtt + ack_delay < latest_rtt):
adjusted_rtt = latest_rtt - ack_delay
adjusted_rtt = max(latest_rtt - ack_delay, min_rtt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the pseudocode that results from #3350, so this change is not editorial

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is a functional change.

Prior to this you get latest_rtt or latest_rtt - ack_delay.

With this change you get min_rtt or latest_rtt - ack_delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups, yes, you are right. I think I got that impression from reading "MUST NOT apply the adjustment if the resulting RTT sample is smaller than the min_rtt", however, that's not what it says.

I think changing the if to the following would be slightly more readable for me but doesn't make a big difference:

if (latest_rtt - ack_delay > min_rtt):

Or even

adjusted_rtt = latest_rtt - ack_delay
if (adjusted_rtt > min_rtt):
adjusted_rtt = latest_rtt

However, we can also just withdraw this PR.

smoothed_rtt = 7/8 * smoothed_rtt + 1/8 * adjusted_rtt
rttvar_sample = abs(smoothed_rtt - adjusted_rtt)
rttvar = 3/4 * rttvar + 1/4 * rttvar_sample
Expand Down