Adding transmission times for every QUIC packet received in the AckFrame.

Adding transmission times for every QUIC packet received in the AckFrame.
Making the scheduler a strict mock for quic connection test.

Merge internal change: 39116450
Merge internal change: 38802607


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171096

[jar (doing other things), 2012-11-29 21:51:40]

These are my comments from last week  (that I didn't send).

I'll try to finish the review now.

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
File net/quic/congestion_control/quic_send_scheduler_test.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
net/quic/congestion_control/quic_send_scheduler_test.cc:53: QuicTime
acc_advance_time;
nit: What does "acc_" mean or stand for?

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
net/quic/congestion_control/quic_send_scheduler_test.cc:82: for (int i = 1; i <=
12; i++) {
nit: better would be named constant (probably here and in line 76) rather than
using 12.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.c...
net/quic/quic_connection.cc:26: // An arbitrary number we'll probably want to
tune.
nit: Please add a comment explaining the intent of the constant, rather than
just saying it is arbitrary.  I had some (mis)understanding with the old name,
but given this transition, I'm left unclear as to the intent (original, or
current).

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.c...
net/quic/quic_connection.cc:170: // We can't have too many acked packets, or our
ack frames go over
nit: This limitation is probably on the count of selectively NACKed packets, so
I don't understand the comment.

Alyssa also advocated for a run-length coding compression for a sequence of
NACKed packets (so that it wouldn't blow the one packet limit), and perhaps that
would be worthy of a TODO.  I could also go and put this into the design doc if
folks would like (i.e., try to define the details).

[jar (doing other things), 2012-11-30 16:50:33]

Some of the indentation is really funky, which both made the code harder to
read, and made me worry that there was a bad merge.

Assuming you feel confident and this code is working, LGTM to land in the
Chromium codebase and work to get the changes corrected in the upstream repo.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
net/quic/quic_connection_test.cc:281: QuicStreamOffset offset, bool fin,
nit: indent

also... (chromium style): put each arg on a separate line, unless they all fit
on one line.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
net/quic/quic_connection_test.cc:342: EXPECT_EQ(1u,
last_frame()->received_info.received_packet_times.size());
Wouldn't it still be reasonable to check the missing_packets.size() as was done
in the original code?

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:369: if (num_acked_packets == 0u) {
Do we really ever send a 0 for the number of acked packets?

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:376: return false;
nit: indent by two more spaces.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:377: }
nit: indent by two more spaces.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:391: return false;
nit: indent by two more spaces.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:392: }
nit: indent by two more spaces.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:590: len += 5 *
(ack.received_info.received_packet_times.size() - 1);
nit: add comment about what is included in this product.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:665: // Special case when no packets are acked, just
transmit the largest.
I'm confused about why this is needed/used.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:668: }
nit: indent last two lines.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:676: }
nit: indent last two lines.

Is there any chance this is the result of a bad merge?... or just an evil
search/replace?

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:689: }
nit: indent last 2 lines by 4 more spaces.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:694: }
nit: indent last two lines.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:180: // The set of all received packets and their
arrival times.
Isn't this really the "trimmed" set? We transmit arrival times at most once, and
should trim this map as we send the times.

[Ryan Hamilton, 2012-12-04 18:04:51]

jar: Thanks for the review.

ianswett:  Since the source of this CL was your internal CL, please cook up an
internal CL to address these comment and add jar@ as the reviewer.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:376: return false;
On 2012/11/30 16:50:33, jar wrote:
> nit: indent by two more spaces.

Weird, this appears to be a merge problem somehow.  Fixed.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:377: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent by two more spaces.

Done.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:391: return false;
On 2012/11/30 16:50:33, jar wrote:
> nit: indent by two more spaces.

Done.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:392: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent by two more spaces.

Done.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:668: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent last two lines.

codereview seems to be confused.  This is correctly indented in the source file.
 *boggle*

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:676: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent last two lines.
> 
> Is there any chance this is the result of a bad merge?... or just an evil
> search/replace?

This is also correct in the file.  Not sure why codereview is confused.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:689: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent last 2 lines by 4 more spaces.

ditto.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:694: }
On 2012/11/30 16:50:33, jar wrote:
> nit: indent last two lines.

ditto

[commit-bot: I haz the power, 2012-12-04 19:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11416155/5012

[commit-bot: I haz the power, 2012-12-04 19:55:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 20:31:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/11416155/5012

[Ian Swett, 2012-12-04 21:40:19]

CL mailed to fix the remaining problems.

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
File net/quic/congestion_control/quic_send_scheduler_test.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
net/quic/congestion_control/quic_send_scheduler_test.cc:53: QuicTime
acc_advance_time;
On 2012/11/29 21:51:40, jar wrote:
> nit: What does "acc_" mean or stand for?  
No idea really, this test was written by Patrik.

https://codereview.chromium.org/11416155/diff/3001/net/quic/congestion_contro...
net/quic/congestion_control/quic_send_scheduler_test.cc:82: for (int i = 1; i <=
12; i++) {
On 2012/11/29 21:51:40, jar wrote:
> nit: better would be named constant (probably here and in line 76) rather than
> using 12.
Done in cleanup cl.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.c...
net/quic/quic_connection.cc:26: // An arbitrary number we'll probably want to
tune.
On 2012/11/29 21:51:40, jar wrote:
> nit: Please add a comment explaining the intent of the constant, rather than
> just saying it is arbitrary.  I had some (mis)understanding with the old name,
> but given this transition, I'm left unclear as to the intent (original, or
> current).

I changed the naming and check back to the old approach and added a comment in
the cleanup CL, but this number seems high to me, even as a starting point. 
Ryan, any thoughts?

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.c...
net/quic/quic_connection.cc:170: // We can't have too many acked packets, or our
ack frames go over
On 2012/11/29 21:51:40, jar wrote:
> nit: This limitation is probably on the count of selectively NACKed packets,
so
> I don't understand the comment.
> 
> Alyssa also advocated for a run-length coding compression for a sequence of
> NACKed packets (so that it wouldn't blow the one packet limit), and perhaps
that
> would be worthy of a TODO.  I could also go and put this into the design doc
if
> folks would like (i.e., try to define the details).

Changed back to the old comment and check in the cleanup CL.   I've added a
TODO.  I'm not sure if now is the right time to  finalize our ack framing with
variable length ints and/or other optimizations, but it may be, since it keeps
coming up and acks are clearly very important.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
net/quic/quic_connection_test.cc:281: QuicStreamOffset offset, bool fin,
On 2012/11/30 16:50:33, jar wrote:
> nit: indent
> 
> also... (chromium style): put each arg on a separate line, unless they all fit
> on one line.
> 
Already fixed in HEAD.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection_t...
net/quic/quic_connection_test.cc:342: EXPECT_EQ(1u,
last_frame()->received_info.received_packet_times.size());
On 2012/11/30 16:50:33, jar wrote:
> Wouldn't it still be reasonable to check the missing_packets.size() as was
done
> in the original code?  
Done in current version now that missing_packets has been restored.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc
File net/quic/quic_framer.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:369: if (num_acked_packets == 0u) {
On 2012/11/30 16:50:33, jar wrote:
> Do we really ever send a 0 for the number of acked packets?

It would seem very odd in practice, and right now we're acking every packet, so
no.  If no packets were recently received, and there were still pending nacks,
would we want to re-send an ack with the nacks?

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:590: len += 5 *
(ack.received_info.received_packet_times.size() - 1);
On 2012/11/30 16:50:33, jar wrote:
> nit: add comment about what is included in this product.
Done in cleanup CL.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_framer.cc#ne...
net/quic/quic_framer.cc:665: // Special case when no packets are acked, just
transmit the largest.
On 2012/11/30 16:50:33, jar wrote:
> I'm confused about why this is needed/used.

Agreed, fixed in HEAD.

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_protocol.h
File net/quic/quic_protocol.h (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_protocol.h#n...
net/quic/quic_protocol.h:180: // The set of all received packets and their
arrival times.
On 2012/11/30 16:50:33, jar wrote:
> Isn't this really the "trimmed" set? We transmit arrival times at most once,
and
> should trim this map as we send the times.
Agreed, comment updated in cleanup CL.

[Ryan Hamilton, 2012-12-04 21:52:54]

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.cc
File net/quic/quic_connection.cc (right):

https://codereview.chromium.org/11416155/diff/3001/net/quic/quic_connection.c...
net/quic/quic_connection.cc:26: // An arbitrary number we'll probably want to
tune.
On 2012/12/04 21:40:19, ianswett wrote:
> On 2012/11/29 21:51:40, jar wrote:
> > nit: Please add a comment explaining the intent of the constant, rather than
> > just saying it is arbitrary.  I had some (mis)understanding with the old
name,
> > but given this transition, I'm left unclear as to the intent (original, or
> > current).
> 
> I changed the naming and check back to the old approach and added a comment in
> the cleanup CL, but this number seems high to me, even as a starting point. 
> Ryan, any thoughts?

Not really.  I suspect that until we have flow control sorted out it might well
be possible to end up with more dropped packets than we might expected, but
honestly I'm not sure what the number threshold is.

[commit-bot: I haz the power, 2012-12-05 00:36:23]

Change committed as 171096

[jar (doing other things), 2012-12-05 00:38:06]

One big thing I didn't pay attention to but should have:

The AckFrame should be distinct from the PacketArrivalTimesFrame.

If you try to put them together, you'll hit limits far too soon.  The must be in
separate frames.

The PacketArrivalTimesFrame will just fit times that it can, and additional
times will be sent in later packets/frames.

This is not clear in the spec, but was explicit in the design doc.

My apologies if I'm misunderstanding the code or intent.

[Ryan Hamilton, 2012-12-05 00:43:07]

On 2012/12/05 00:38:06, jar wrote:
> One big thing I didn't pay attention to but should have:
> 
> The AckFrame should be distinct from the PacketArrivalTimesFrame.
> 
> If you try to put them together, you'll hit limits far too soon.  The must be
in
> separate frames.
> 
> The PacketArrivalTimesFrame will just fit times that it can, and additional
> times will be sent in later packets/frames.
> 
> This is not clear in the spec, but was explicit in the design doc.
> 
> My apologies if I'm misunderstanding the code or intent.

Since this code has now landed with the packet arrival times in the ack frame,
we should continue this discussion in a different venue.  Perhaps the CL ian is
working on?

[jar (doing other things), 2012-12-05 00:47:18]

Yes... I just added comments in the upstream CL.

On Tue, Dec 4, 2012 at 4:43 PM, <rch@chromium.org> wrote:

> On 2012/12/05 00:38:06, jar wrote:
>
>> One big thing I didn't pay attention to but should have:
>>
>
>  The AckFrame should be distinct from the PacketArrivalTimesFrame.
>>
>
>  If you try to put them together, you'll hit limits far too soon.  The
>> must be
>>
> in
>
>> separate frames.
>>
>
>  The PacketArrivalTimesFrame will just fit times that it can, and
>> additional
>> times will be sent in later packets/frames.
>>
>
>  This is not clear in the spec, but was explicit in the design doc.
>>
>
>  My apologies if I'm misunderstanding the code or intent.
>>
>
> Since this code has now landed with the packet arrival times in the ack
> frame,
> we should continue this discussion in a different venue.  Perhaps the CL
> ian is
> working on?
>
>
https://codereview.chromium.**org/11416155/<https://codereview.chromium.org/1...
>