For packet pacing and non-paced network connectivity tests, collect

For packet pacing and non-paced network connectivity tests, collect
the bit vector for the first 6 packets received.

For example, among the first 6 packets, if all packets other than
packet 2 and 4 are responded to, then we'd have a sample (in binary)
of 110101B, or 53.

R=jar
TEST=browser unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156987

[ramant (doing other things), 2012-09-14 22:05:11]

Hi Jim,
  Would appreciate your comments. Will add the new histograms to histograms.xml
in a separate CL.

thanks much
raman

[jar (doing other things), 2012-09-14 22:34:54]

This should be very helpful!  Thanks!

I think since you overlapped the code for the 6 packet test, you can remove a
bunch of stuff for what used to be a stand-alone 6-packet test.  I searched for
refs to the count, to try to spot the points... and added comments.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
File chrome/browser/net/network_stats.cc (right):

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:87: // Maximum number of packets that can be
sent to the server for packet loss
nit:  Change comment to:

Number of packets that are recorded in a packet-corelation histogram, which
shows exactly what seqence of packets were responded to.  We use this to deduce
specific packet loss correlation.


You might also change the name to: kCorrelatedLossPacketCount.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:686: packets_to_send_ !=
kMaximumCorrelationPackets) {
This line can probably be deleted (you can leave 685).

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:694: return;
No longer needed.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:796: uint32 six_packets_received_mask =
nit: suggest correlated_packet_mask

This way, the size is held in the kMaximumCorrelationPackets only.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:807: 2 << kMaximumCorrelationPackets,
I think this and the next line can use (with plus 1 on next line):

1 << kMaximumCorrelationPackets

It will shrink the histograms.... but you weren't using the high end ones.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:816: 1, kMaximumCorrelationPackets,
kMaximumCorrelationPackets + 1,
We might not need this anymore, as we'll get it as part of the packets_acked
histograms (for the first 6? packets).

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:1016: kLargeTestBytesToSend,
kMaximumCorrelationPackets,
We probably don't need this test anymore.  Line 1009 can reduce the choices as
well.

[ramant (doing other things), 2012-09-14 23:39:08]

Many many thanks for your comments. Made all the changes you have suggested.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
File chrome/browser/net/network_stats.cc (right):

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:87: // Maximum number of packets that can be
sent to the server for packet loss
On 2012/09/14 22:34:54, jar wrote:
> nit:  Change comment to:
> 
> Number of packets that are recorded in a packet-corelation histogram, which
> shows exactly what seqence of packets were responded to.  We use this to
deduce
> specific packet loss correlation.
> 
> 
> You might also change the name to: kCorrelatedLossPacketCount.

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:686: packets_to_send_ !=
kMaximumCorrelationPackets) {
On 2012/09/14 22:34:54, jar wrote:
> This line can probably be deleted (you can leave 685).

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:694: return;
On 2012/09/14 22:34:54, jar wrote:
> No longer needed.

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:796: uint32 six_packets_received_mask =
On 2012/09/14 22:34:54, jar wrote:
> nit: suggest correlated_packet_mask
> 
> This way, the size is held in the kMaximumCorrelationPackets only.

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:807: 2 << kMaximumCorrelationPackets,
On 2012/09/14 22:34:54, jar wrote:
> I think this and the next line can use (with plus 1 on next line):
> 
> 1 << kMaximumCorrelationPackets
> 
> It will shrink the histograms.... but you weren't using the high end ones.

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:816: 1, kMaximumCorrelationPackets,
kMaximumCorrelationPackets + 1,
On 2012/09/14 22:34:54, jar wrote:
> We might not need this anymore, as we'll get it as part of the packets_acked
> histograms (for the first 6? packets).

Done.

https://chromiumcodereview.appspot.com/10933101/diff/1/chrome/browser/net/net...
chrome/browser/net/network_stats.cc:1016: kLargeTestBytesToSend,
kMaximumCorrelationPackets,
On 2012/09/14 22:34:54, jar wrote:
> We probably don't need this test anymore.  Line 1009 can reduce the choices as
> well.

Done.

[jar (doing other things), 2012-09-15 00:50:24]

lgtm

[commit-bot: I haz the power, 2012-09-15 00:53:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10933101/2003

[commit-bot: I haz the power, 2012-09-15 05:08:08]

Change committed as 156987