Subject: [quicwg/base-drafts] Simplify UpdateRtt pseudocode (#2610)

From: Jana Iyengar <[email-masked]>
Date: Fri, 12 Apr 2019 05:24:26 +0000 (UTC)

You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2610

-- Commit Summary --

  * Simplify UpdateRtt pseudocode

-- File Changes --

    M draft-ietf-quic-recovery.md (22)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2610.patch
https://github.com/quicwg/base-drafts/pull/2610.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610

---

From: Lars Eggert <[email-masked]>
Date: Thu, 11 Apr 2019 22:41:23 -0700

larseggert commented on this pull request.

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

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#pullrequestreview-225903555

---

From: Lars Eggert <[email-masked]>
Date: Thu, 11 Apr 2019 22:44:52 -0700

> @@ -1101,22 +1101,24 @@ OnAckReceived(ack, pn_space):

 UpdateRtt(latest_rtt, ack_delay):

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#pullrequestreview-225904179

---

From: Lars Eggert <[email-masked]>
Date: Thu, 11 Apr 2019 22:45:51 -0700

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#discussion_r274766118

---

From: Lars Eggert <[email-masked]>
Date: Thu, 11 Apr 2019 22:48:16 -0700

UpdateRtt(latest_rtt, ack_delay):
+  if (smoothed_rtt == 0):
+    // First RTT sample.

Shouldn't you adjust by `ack_delay` here?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#pullrequestreview-225904758

---

From: Martin Thomson <[email-masked]>
Date: Thu, 11 Apr 2019 23:52:19 -0700

martinthomson approved this pull request.

This looks good.

latest_rtt probably shouldn't be global.

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

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#pullrequestreview-225919515

---

From: Marten Seemann <[email-masked]>
Date: Fri, 12 Apr 2019 08:40:24 +0000 (UTC)

marten-seemann commented on this pull request.

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#discussion_r274812476

---

From: ianswett <[email-masked]>
Date: Fri, 12 Apr 2019 14:31:53 +0000 (UTC)

ianswett approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#pullrequestreview-226114530

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 12 Apr 2019 18:34:21 -0700

Merged #2610 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2610#event-2273865216