Subject: [quicwg/base-drafts] Replace magic numbers with constants and define RTT calculations (#149)

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 09:13:01 -0800

Defines how variables such as smoothed_rtt and rttvar are calculated, as well as replacing many numbers with constants.

Intends to fix #107
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Replace magic numbers with constants and define RTT calculations

-- File Changes --

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

-- Patch Links --

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

---

From: janaiyengar <[email-masked]>
Date: Fri, 13 Jan 2017 10:57:37 -0800

janaiyengar approved this pull request.

one nit, but LGTM

> @@ -188,6 +188,32 @@ latency before a userspace QUIC receiver processes a received packet.
 We now describe QUIC's loss detection as functions that should be called on
 packet transmission, when a packet is acked, and timer expiration events.
 
+## Constants of interest
+
+Constants used in loss recovery and congestion control are based on a combination
+of RFCs, papers, and common practice.  Some may need to be changed or negotiated
+in order to better suit a variety of environments.  
+

We should also justify these constants. I'll create an issue to do this.

> @@ -287,8 +322,41 @@ Pseudocode for OnPacketSent follows:
 
 ~~~
  OnPacketSent(packet_number, is_retransmittable):
-     if is_retransmittable:
-       SetLossDetectionAlarm()
+   # TODO: Clarify the data in sent_packets.
+   sent_packets[packet_number] = {now}
+   if is_retransmittable:
+     SetLossDetectionAlarm()
+~~~
+
+## On Ack Receipt
+
+When an ack is received, it may acknowledge 0 or more packets.  
+
+Pseudocode for OnAckReceied and UpdateRtt follow:

nit: "received"

-- 
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/149#pullrequestreview-16627640

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 11:42:21 -0800

ianswett commented on this pull request.

 ~~~

Done

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 11:43:39 -0800

@ianswett pushed 1 commit.

ec152c8  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/149/files/ee514e298dbc3c39b749c14d6c95fdf84557fb7b..ec152c8b18905e342cb55ca10262a155bc700459

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 12:39:19 -0800

b330104  Remove trailing whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/149/files/ec152c8b18905e342cb55ca10262a155bc700459..b330104ebeb3861c70e291e89549868f18de6f14

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 13:08:56 -0800

b1ef5f0  More trailing whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/149/files/b330104ebeb3861c70e291e89549868f18de6f14..b1ef5f0e6beb214fd352f06acc7c66a8ba5431b6

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 13:18:22 -0800

bf83371  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/149/files/b1ef5f0e6beb214fd352f06acc7c66a8ba5431b6..bf833715a118c7ee70f044e78826a40f674c55fe

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 13:29:38 -0800

64a4ac2  Whitespace

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/149/files/bf833715a118c7ee70f044e78826a40f674c55fe..64a4ac263704b841afd0dac7c094d92dd62a9adf

---

From: ianswett <[email-masked]>
Date: Fri, 13 Jan 2017 13:36:14 -0800

Merged #149.

-- 
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/149#event-922866269

---

From: Martin Thomson <[email-masked]>
Date: Sun, 15 Jan 2017 13:15:57 -0800

martinthomson commented on this pull request.

 ~~~

why braces here, but not below (Line 341)?

-- 
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/149#pullrequestreview-16723680

---

From: Martin Thomson <[email-masked]>
Date: Sun, 15 Jan 2017 20:05:48 -0800

> +       UpdateRtt(rtt_sample)
+     // Find all newly acked packets.
+     for acked_packet in DetermineNewlyAckedPackets():
+       OnPacketAcked(acked_packet)
+
+     DetectLostPackets(ack.largest_acked_packet);
+     SetLossDetectionAlarm();
+
+
+   UpdateRtt(rtt_sample):
+     if (smoothed_rtt == 0):
+       smoothed_rtt = rtt_sample
+       rttvar = rtt_sample / 2
+     else:
+       rttvar = 3/4 * rttvar + 1/4 * (smoothed_rtt - rtt_sample)
+       smoothed_rtt = 7/8 * smoothed_rtt + 1/8 * rtt_sample

\[citation-needed]

-- 
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/149#pullrequestreview-16738072

---

From: Martin Thomson <[email-masked]>
Date: Sun, 15 Jan 2017 20:08:31 -0800

Oh, I see that this is just RFC 6298 directly implemented.  Might pay to be more direct about that.  Only the smoothed_rtt value contains the reference.