Subject: [quicwg/base-drafts] editorial: simplify pseudo code (recovery) (#3384)

From: mirjak <[email-masked]>
Date: Wed, 22 Jan 2020 08:50:20 -0800

Btw. shouldn&#39;t the weights be parameters and not fixed values (of 1/8 and 1/4)? And where does those values come from?
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * editorial: simplify pseudo code (recovery)

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3384.patch
https://github.com/quicwg/base-drafts/pull/3384.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/3384

---

From: ianswett <[email-masked]>
Date: Wed, 22 Jan 2020 15:18:24 -0800

ianswett requested changes on this pull request.

PTAL at https://github.com/quicwg/base-drafts/issues/3350

> @@ -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)

This is the pseudocode that results from https://github.com/quicwg/base-drafts/issues/3350, so this change is not editorial

-- 
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/3384#pullrequestreview-346971460

---

From: Martin Thomson <[email-masked]>
Date: Wed, 22 Jan 2020 15:51:10 -0800

martinthomson commented on this pull request.

 ~~~

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`.

-- 
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/3384#discussion_r369868333

---

From: mirjak <[email-masked]>
Date: Thu, 23 Jan 2020 01:46:11 -0800

mirjak commented on this pull request.

 ~~~

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.

-- 
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/3384#discussion_r370017839