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

Simplify UpdateRtt pseudocode #2610

Merged
merged 1 commit into from Apr 13, 2019
Merged

Simplify UpdateRtt pseudocode #2610

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
22 changes: 12 additions & 10 deletions draft-ietf-quic-recovery.md
View file
Open in desktop
Expand Up @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space):


UpdateRtt(latest_rtt, ack_delay):
Copy link
Member

Choose a reason for hiding this comment

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

Why is latest_rtt passed in as a parameter? It's already a global. Does this parameter differ from the global?

Copy link
Member

Choose a reason for hiding this comment

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

latest_rtt probably shouldn't be global.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (smoothed_rtt == 0):
// First RTT sample.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you adjust by ack_delay here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. #2607 covers this though.

min_rtt = latest_rtt
smoothed_rtt = latest_rtt
rttvar = latest_rtt / 2
return

// min_rtt ignores ack delay.
min_rtt = min(min_rtt, latest_rtt)
// Limit ack_delay by max_ack_delay
ack_delay = min(ack_delay, max_ack_delay)
// Adjust for ack delay if it's plausible.
// Adjust for ack delay if plausible.
adjusted_rtt = latest_rtt
if (latest_rtt - min_rtt > ack_delay):
if (latest_rtt > min_rtt + ack_delay):
adjusted_rtt = latest_rtt - ack_delay
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

(But if you want to use the global here, maybe that adjustment to the global is wrong?)

Copy link
Member

Choose a reason for hiding this comment

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

The prose relies on latest_rtt in descriptions. Adjusting that value directly would confuse things.

// First RTT sample.
if (smoothed_rtt == 0):
smoothed_rtt = latest_rtt
rttvar = latest_rtt / 2
else:
rttvar_sample = abs(smoothed_rtt - adjusted_rtt)
rttvar = 3/4 * rttvar + 1/4 * rttvar_sample
smoothed_rtt = 7/8 * smoothed_rtt + 1/8 * adjusted_rtt

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


Expand Down