[quicwg/base-drafts] 587f8a: Refactor DetectLostPackets

janaiyengar <jri.ietf@gmail.com> Thu, 06 December 2018 23:54 UTC

Sender: jri.ietf=gmail.com@github.com
Date: Thu, 06 Dec 2018 15:54:25 -0800
From: janaiyengar <jri.ietf@gmail.com>
Reply-To: janaiyengar <jri.ietf@gmail.com>
To: quic-issues@ietf.org
Message-ID: <5c09b6b1828cc_44ed2ad319c5057862296@hookshot-fe-6e9b612.cp1-iad.github.net.mail>
Subject: [quicwg/base-drafts] 587f8a: Refactor DetectLostPackets
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="--==_mimepart_5c09b6b1824a8_44ed2ad319c505786217f"; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/X7JmeymxDRDsD3VPHcj1UxkQ7Fo>
Precedence: list

  Branch: refs/heads/master
  Home:   https://github.com/quicwg/base-drafts
  Commit: 587f8a5b72824b73b0543a33841a6437ceb0b981
      https://github.com/quicwg/base-drafts/commit/587f8a5b72824b73b0543a33841a6437ceb0b981
  Author: Martin Thomson <martin.thomson@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Refactor DetectLostPackets

This had a couple of problems that I think this addresses, and some that
it doesn't.

The first is that it isn't clear what is being iterated over and in what
order.  I think that the point here is to iterate over sent_packets,
starting with the oldest.  Correct me if that is wrong.  In any case,
this change doesn't assume an iteration order.  The only reason the
iteration order is important is in setting loss_time, which needs to be
the earliest time (assuming that I'm right).

This simplifies the function, by setting thresholds at the top and doing
a simple comparison.

I've added a note about loss_time potentially being in the past.

The problem that remains is that this appeared to iterate only over
packets that have a packet number less than the largest acknowledged.
I've added that condition to the loop, but I don't think that it's
right.  I think that it's just redundant - and while an implementation
might stop its iteration at the largest acknowledged to save cycles,
this function will operate the same without the extra check.

It's also not clear whether the greater than comparisons here were
correct.  If you assume that firing of the timer cannot take zero time,
this is never an issue, but with discrete intervals on time values,
that's not always going to happen.  As setup, this code could be called
at exactly loss_time for a packet, in which case that packet will not be
declared lost.  I think that this wants >= in the time comparison for
that reason.

Finally, should the early retransmit timer be configurable?  Should it
be set based on kTimeReorderingThreshold?


  Commit: 29f5fdf72186f71d0d34d487dac8b18a30584e13
      https://github.com/quicwg/base-drafts/commit/29f5fdf72186f71d0d34d487dac8b18a30584e13
  Author: Martin Thomson <martin.thomson@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Review changes


  Commit: db6929a33d31de152198f6560aad31a0ed53f359
      https://github.com/quicwg/base-drafts/commit/db6929a33d31de152198f6560aad31a0ed53f359
  Author: Martin Thomson <martin.thomson@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Use <= for cutoffs


  Commit: 02a164ee1e8e208e414896124c15c627f79dcea8
      https://github.com/quicwg/base-drafts/commit/02a164ee1e8e208e414896124c15c627f79dcea8
  Author: Jana Iyengar <jri.ietf@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Fixes some broken bits


  Commit: 71e06d391739652c1ed965873f530bb496f8aecd
      https://github.com/quicwg/base-drafts/commit/71e06d391739652c1ed965873f530bb496f8aecd
  Author: Jana Iyengar <jri.ietf@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  lint


  Commit: cdaef0e2036dc9da2c972a9ec0d2c795559c3206
      https://github.com/quicwg/base-drafts/commit/cdaef0e2036dc9da2c972a9ec0d2c795559c3206
  Author: Jana Iyengar <jri.ietf@gmail.com>
  Date:   2018-12-05 (Wed, 05 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  cleanup and consistent parens


  Commit: e50cf51845fdfb8357e974bdc2ad4134d3829663
      https://github.com/quicwg/base-drafts/commit/e50cf51845fdfb8357e974bdc2ad4134d3829663
  Author: ianswett <ianswett@users.noreply.github.com>
  Date:   2018-12-06 (Thu, 06 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Update draft-ietf-quic-recovery.md

Move and update Martin's comments


  Commit: f29543ca5d015c8bf3f6ff0d905892bbf87562e2
      https://github.com/quicwg/base-drafts/commit/f29543ca5d015c8bf3f6ff0d905892bbf87562e2
  Author: janaiyengar <jri.ietf@gmail.com>
  Date:   2018-12-06 (Thu, 06 Dec 2018)

  Changed paths:
    M draft-ietf-quic-recovery.md

  Log Message:
  -----------
  Merge pull request #2066 from quicwg/refactor-loss_time

Refactor DetectLostPackets


Compare: https://github.com/quicwg/base-drafts/compare/1bc6191ec681...f29543ca5d01
      **NOTE:** This service has been marked for deprecation: https://developer.github.com/changes/2018-04-25-github-services-deprecation/

      Functionality will be removed from GitHub.com on January 31st, 2019.

[quicwg/base-drafts] 587f8a: Refactor DetectLostP… janaiyengar