2 experiments: DNS prefetch limit concurrency: TCP split a packet

2 experiments: DNS prefetch limit concurrency: TCP split a packet

Some firewalls apparently try to preclude a "syn flood to host" by limiting
the number of syn's (used to open a TCP/IP socket) that are outstanding
without having received a syn-ack.  Presumably this is to prevent a user
from participating in a syn-flood attack (which traditional sends a lot
of syn packets, with false return addresses, resulting in no responses).

Apparently this firewall technology has in some cases been extended
to include UDP sessions for which there has been no response, and this
may include DNS resolutions.  Since the prefetcher currently resolves
as many as 8 names simultaneously, this is remarkably close to the 
reported threshold of 10 un-answered connections.  This test attempts
to limit connections to 2, 4, or 6, so that we can see if this helps
users.

In TCP, the RTO remains (under windows) at a full 3 seconds until after the
first ack is received.  As a result, if the first data packet sent (after
the SYN) is lost, then TCP won't resend until after 3 seconds without an ack.
As a test, we split up the first packet into two parts (the second part
containing only one byte).  This is done as an A/B test, and we'll see
if we get a measurable improvement in page-load-time latency.

Finally, to get better page load stats, I adjusted the PLT histograms
so that we record a "final" time for abandoned pages when they are
closed (even if they didn't finish rendering, etc.).  This should give
a much more fair PLT comparison for all network latency experiments.


BUG=3041
BUG=12754
r=mbelshe,darin

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

[jar (doing other things), 2010-03-19 16:51:36]

Mike: Please look at the A/B test in http_stream_parser.cc.  This is an
implementation as per our discussion.  We could make it better in the future
(only split on warm sockets), but hopefully we'll get a nice experimental
result.

Darin: Can you look at render_view.cc?  I'm trying to capture the event where a
page is abandoned, and I'm going to "charge" the user as though the PLT
completes at that abandonment point.  This avoids discarding stats for abandoned
pages.  I just want to be sure the abandonment status isn't prematurely
triggered in the code (i.e., we don't routinely call DumpHistograms in the file
a multitude of times).

Mike: Please also have a look at the rest of the code.  It is pretty routine
(gathering of histograms, analogously to other experiments).  There is tiny
fragment (that I had sitting in dns_global) where I avoid doing prefetches on
things like foo.co as the user types foo.com.  I could remove that form this CL
if it looks too confusing... but all it can ever do is diminish the amount of
prefetching, so it should be very safe.

[Mike Belshe, 2010-03-19 17:42:28]

http://codereview.chromium.org/1088002/diff/15004/4006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4006#newcode438
chrome/browser/browser_main.cc:438: bool kSplitPacketFieldTrial = true;
nit: I find this extra if statement wasteful.  If you want a scope for defining
your stuff you could just use 
{
}

http://codereview.chromium.org/1088002/diff/15004/4006#newcode443
chrome/browser/browser_main.cc:443: FieldTrial::Probability kProbabilityPerGroup
= 50;  // 50% probability.
nit: do we want to experiment with 50% of our users this way?  Why not 5%?

http://codereview.chromium.org/1088002/diff/15004/4003
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4003#newcode126
chrome/browser/net/dns_global.cc:126: std::string host = url.host();
A comment about why you've chosen this heuristic for skipping out on the
prefetch might be nice.

http://codereview.chromium.org/1088002/diff/15004/4005
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4146
chrome/renderer/render_view.cc:4146: WebFrame* main_frame =
webview()->mainFrame();
Larger complaint:  the number of histograms getting dumped here is getting
pretty large; we need to figure out a better architecture for multi-variable
histograms.  (I count 54 histograms generated in this function - mostly with
redundant data for different cases)

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4162
chrome/renderer/render_view.cc:4162: NavigationState::kLoadTypeMax);
I think the max should be 2?

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4287
chrome/renderer/render_view.cc:4287: abandoned_page ? 1 : 0,
NavigationState::kLoadTypeMax);
same bug - max should be 2?

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4289
chrome/renderer/render_view.cc:4289: FieldTrial::MakeName("Renderer4.LoadType",
"DnsImpact"),
I'm not quite sure why you're doing a field-trial-specific version of this
histogram; do loadtypes change due to this?  I guess you're looking for the user
doing more RELOADs?

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4326
chrome/renderer/render_view.cc:4326: abandoned_page ? 1 : 0,
NavigationState::kLoadTypeMax);
max = 2

http://codereview.chromium.org/1088002/diff/15004/4004
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4004#newcode192
net/http/http_stream_parser.cc:192: // at least on Windows... but with two
packets, the probability of loss
note: I suspect this is a WinXP problem only; but I haven't got confirmation of
that.

http://codereview.chromium.org/1088002/diff/15004/4004#newcode199
net/http/http_stream_parser.cc:199: DCHECK_EQ(trial->group_name(),
"_first_packet_split");
nits: ditch the {} and spacing is off.

http://codereview.chromium.org/1088002/diff/15004/4004#newcode202
net/http/http_stream_parser.cc:202: int bytes_remaining =
request_headers_->BytesRemaining();
note: This is going to split the packet for every request sent; not just the
first request on the channel.

[jar (doing other things), 2010-03-19 19:54:30]

Changes made per suggestions by mbelshe.

http://codereview.chromium.org/1088002/diff/15004/4006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4006#newcode438
chrome/browser/browser_main.cc:438: bool kSplitPacketFieldTrial = true;
I was trying to make it real easy to disable.... but I'll just remove the if per
your suggestion.

On 2010/03/19 17:42:28, Mike Belshe wrote:
> nit: I find this extra if statement wasteful.  If you want a scope for
defining
> your stuff you could just use 
> {
> }

http://codereview.chromium.org/1088002/diff/15004/4006#newcode443
chrome/browser/browser_main.cc:443: FieldTrial::Probability kProbabilityPerGroup
= 50;  // 50% probability.
On 2010/03/19 17:42:28, Mike Belshe wrote:
> nit: do we want to experiment with 50% of our users this way?  Why not 5%?

I wasn't expecting anything negative in either case... and wanted to be sure I
quickly got statistically significant data, even on Mac and Linux (with their
low populations).  I split the difference, and pushed this experiment down to
10%.

http://codereview.chromium.org/1088002/diff/15004/4003
File chrome/browser/net/dns_global.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4003#newcode126
chrome/browser/net/dns_global.cc:126: std::string host = url.host();
On 2010/03/19 17:42:28, Mike Belshe wrote:
> A comment about why you've chosen this heuristic for skipping out on the
> prefetch might be nice.

I just reverted this part of the CL.  It should really land with more intense
comments (both inline, and in the CL) about why this is reasonable.  (it is also
not as critical or timely as the rest of the CL).

http://codereview.chromium.org/1088002/diff/15004/4005
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4146
chrome/renderer/render_view.cc:4146: WebFrame* main_frame =
webview()->mainFrame();
On 2010/03/19 17:42:28, Mike Belshe wrote:
> Larger complaint:  the number of histograms getting dumped here is getting
> pretty large; we need to figure out a better architecture for multi-variable
> histograms.  (I count 54 histograms generated in this function - mostly with
> redundant data for different cases)

Larger answer:

We have soooo many stats because the PLT metrics were really not quite correct,
and the extra metrics are in large part exploratory.  Most of them should go
away.

...and...

We have several tests going on here.  When there are a lot of PLT related tests,
this file should swell up... and when then they should be removed when they are
no longer active tests (even though the code is crafted to skip sections with
inactive trials).  

Bottom line: When the PLT is really stable (reliable? understandable?
meaningful?) we'll get done with several of these experiments... and we won't
have the extra stats floating around (and certainly, we won't have them surfaced
in A/B tests).

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4162
chrome/renderer/render_view.cc:4162: NavigationState::kLoadTypeMax);
On 2010/03/19 17:42:28, Mike Belshe wrote:
> I think the max should be 2?

Done.

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4287
chrome/renderer/render_view.cc:4287: abandoned_page ? 1 : 0,
NavigationState::kLoadTypeMax);
On 2010/03/19 17:42:28, Mike Belshe wrote:
> same bug - max should be 2?

Done.

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4289
chrome/renderer/render_view.cc:4289: FieldTrial::MakeName("Renderer4.LoadType",
"DnsImpact"),
On 2010/03/19 17:42:28, Mike Belshe wrote:
> I'm not quite sure why you're doing a field-trial-specific version of this
> histogram; do loadtypes change due to this?  I guess you're looking for the
user
> doing more RELOADs?

This is the ongoing battle with Simpson's Paradox.  We need to study each of the
various load-type distributions separately, to be sure they are all improving.  

These ENUMERATION histograms tend to show the "happiness metric" by looking at
percentage of abandonment, vs willing to wait (line 4286), as well as the
relative distribution of link-following, vs stop/reload, vs back.  The
link-load-cache-stale is mostly used by back (and forward), but is also used at
startup to resurrect a tab.

http://codereview.chromium.org/1088002/diff/15004/4005#newcode4326
chrome/renderer/render_view.cc:4326: abandoned_page ? 1 : 0,
NavigationState::kLoadTypeMax);
On 2010/03/19 17:42:28, Mike Belshe wrote:
> max = 2

Done.

http://codereview.chromium.org/1088002/diff/15004/4004
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/1088002/diff/15004/4004#newcode192
net/http/http_stream_parser.cc:192: // at least on Windows... but with two
packets, the probability of loss
On 2010/03/19 17:42:28, Mike Belshe wrote:
> note: I suspect this is a WinXP problem only; but I haven't got confirmation
of
> that.

I think the idea is interesting enough that I'm still game to explore it (via
this test).  As noted, I pulled the percentage down to 10 percent of runs.

http://codereview.chromium.org/1088002/diff/15004/4004#newcode199
net/http/http_stream_parser.cc:199: DCHECK_EQ(trial->group_name(),
"_first_packet_split");
On 2010/03/19 17:42:28, Mike Belshe wrote:
> nits: ditch the {} and spacing is off.

Done.

http://codereview.chromium.org/1088002/diff/15004/4004#newcode202
net/http/http_stream_parser.cc:202: int bytes_remaining =
request_headers_->BytesRemaining();
On 2010/03/19 17:42:28, Mike Belshe wrote:
> note: This is going to split the packet for every request sent; not just the
> first request on the channel.

You did too good a job... and reviewed too fast... Will had told me where to
find the reuse status, and this is fixed in the newest snapshot (and my comment
about this on line 195 is also removed).

[Mike Belshe, 2010-03-19 23:50:25]

lgtm

http://codereview.chromium.org/1088002/diff/2007/48002
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/1088002/diff/2007/48002#newcode196
net/http/http_stream_parser.cc:196: static const bool kEnsureSecondPacket(kTrial
&& (kTrial->group() == 0));
very minor nit:  I had to read this twice, the name "kEnforceSecondPacket" is
really "kForceSecondPacket".  What do you think?

[jar (doing other things), 2010-03-19 23:54:42]

Rename done per Mike's suggestion.

http://codereview.chromium.org/1088002/diff/2007/48002
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/1088002/diff/2007/48002#newcode196
net/http/http_stream_parser.cc:196: static const bool kEnsureSecondPacket(kTrial
&& (kTrial->group() == 0));
On 2010/03/19 23:50:25, Mike Belshe wrote:
> very minor nit:  I had to read this twice, the name "kEnforceSecondPacket" is
> really "kForceSecondPacket".  What do you think?

Done.

[darin (slow to review), 2010-03-27 04:32:47]

sorry for missing this code review.  the render_view.cc changes lgtm.