Deciding best connection to schedule requests on based on cwnd and idle time

Warmth of a connection (cwnd) is estimated by the amount of data written to the socket. 
Choosing the warmest connection would mean faster resource load times. 

idle time is the time a socket has remained idle (no http requests being served on it). 
Probability of server resetting a connection increases with idle time duration. 

Using a cost function that takes into account bytes transferred and idle time to pick best connection to schedule http requests on.

CODEREVIEW done in http://codereview.chromium.org/6990036/

Contributed by gagansingh@google.com

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

[willchan no longer on Chromium, 2011-06-19 15:12:47]

LGTM

[commit-bot: I haz the power, 2011-06-19 15:29:13]

Try job failure for 7189055-3006 on win for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-06-19 20:06:28]

Try job failure for 7189055-3029 on win for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-06-21 05:38:08]

Can't apply patch for file chrome/browser/browser_main.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/browser_main.cc
Hunk #4 FAILED at 667.
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/browser_main.cc.rej

[commit-bot: I haz the power, 2011-06-21 09:35:42]

Presubmit check for 7189055-11009 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/p2p/socket_host_test_utils.h

Presubmit checks took 2.1s to calculate.

[willchan no longer on Chromium, 2011-06-21 09:38:33]

Adding sergeyu

[Sergey Ulanov, 2011-06-21 17:25:15]

remoting/*, jingle/*, and content/browser/renderer_host/p2p/* - LGTM.

http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h#...
net/socket/stream_socket.h:86: virtual int GetConnectTimeMicros() const = 0;
Maybe use base::TimeDelta instead of int here?

[commit-bot: I haz the power, 2011-06-22 07:31:38]

Can't process patch for file net/http/http_proxy_client_socket.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-06-22 07:57:43]

Try job failure for 7189055-16001 on win for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2011-06-22 08:31:06]

Try job failure for 7189055-18008 on linux for step "net_unittests".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[willchan no longer on Chromium, 2011-06-22 09:18:56]

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:413: DCHECK_GE(num_kb, 0);
Please don't just remove DCHECKs. They're there for a reason. You need to
justify why you remove them.

Looking at the error: "Check failed: num_kb >= 0 (-0.000976562 vs. 0)".

That value is -1 / 1024. Why is bytes_read equal to -1? That looks wrong/bad.

[gagansingh, 2011-06-22 11:10:02]

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:413: DCHECK_GE(num_kb, 0);
On 2011/06/22 09:18:57, willchan wrote:
> Please don't just remove DCHECKs. They're there for a reason. You need to
> justify why you remove them.
> 
> Looking at the error: "Check failed: num_kb >= 0 (-0.000976562 vs. 0)".
> 
> That value is -1 / 1024. Why is bytes_read equal to -1? That looks wrong/bad.

The problem was that NumBytesRead method defined
client_socket_pool_base_unittest returned -1, which is why this DCHECK failed.
Looking at MockClientSocket, it seems it doesn't expect Read() calls, so
returning -1 there seems okay.
Hence removing this DCHECK.
What do you have in mind ?

[M-A Ruel, 2011-06-22 14:53:32]

Drive by.

http://codereview.chromium.org/7189055/diff/19015/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7189055/diff/19015/chrome/browser/browser_main...
chrome/browser/browser_main.cc:212: void SetSocketReusePolicy(int
warmest_socket_trial_group,
smells like:

const int* result = std::find(socket_policy, socket_policy + num_groups,
warmest_socket_trial_group);
DCHECK_NE(result, socket_policy + num_groups);
net::SetSocketReusePolicy(*result);

?

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:39: double
g_socket_reuse_policy_penalty_exponent = -1;
!!!!!

Do not use global variable there. There was not thread safe.

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:57: // NOTE: 'policy' should be a valid
ClientSocketReusePolicy enum value.
Then why isn't it a ClientSocketReusePolicy?

Why is this function global?

[willchan no longer on Chromium, 2011-06-22 18:39:03]

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/18008/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:413: DCHECK_GE(num_kb, 0);
On 2011/06/22 11:10:02, gagansingh wrote:
> On 2011/06/22 09:18:57, willchan wrote:
> > Please don't just remove DCHECKs. They're there for a reason. You need to
> > justify why you remove them.
> > 
> > Looking at the error: "Check failed: num_kb >= 0 (-0.000976562 vs. 0)".
> > 
> > That value is -1 / 1024. Why is bytes_read equal to -1? That looks
wrong/bad.
> 
> The problem was that NumBytesRead method defined
> client_socket_pool_base_unittest returned -1, which is why this DCHECK failed.
> Looking at MockClientSocket, it seems it doesn't expect Read() calls, so
> returning -1 there seems okay.
> Hence removing this DCHECK.
> What do you have in mind ?

Sorry, I just realized that you have absolutely no tests for this code. You're
just using -1 in NumBytesRead(). Please write tests.

[gagansingh, 2011-06-23 15:53:58]

Hi Will

Added relevant tests, am syncing and putting this on try server, will let you
know once try passes.

Thanks
Gagan

http://codereview.chromium.org/7189055/diff/19015/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7189055/diff/19015/chrome/browser/browser_main...
chrome/browser/browser_main.cc:212: void SetSocketReusePolicy(int
warmest_socket_trial_group,
On 2011/06/22 14:53:33, Marc-Antoine Ruel wrote:
> smells like:
> 
> const int* result = std::find(socket_policy, socket_policy + num_groups,
> warmest_socket_trial_group);
> DCHECK_NE(result, socket_policy + num_groups);
> net::SetSocketReusePolicy(*result);
> 
> ?

Done.

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:39: double
g_socket_reuse_policy_penalty_exponent = -1;
On 2011/06/22 14:53:33, Marc-Antoine Ruel wrote:
> !!!!!
> 
> Do not use global variable there. There was not thread safe.

There were lot of review comments on this one, this is what willchan and mbelshe
finally settled on (see http://codereview.chromium.org/6990036/ for more review
comments).

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.h (right):

http://codereview.chromium.org/7189055/diff/21022/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:57: // NOTE: 'policy' should be a valid
ClientSocketReusePolicy enum value.
On 2011/06/22 14:53:33, Marc-Antoine Ruel wrote:
> Then why isn't it a ClientSocketReusePolicy?
> 
> Why is this function global?

I'm avoiding an ugly looking if condition by assuming that the value is a valid
ClientSocketReusePolicy enum.
Function is global and NET_API because browser_main needs to set the value.

[gagansingh, 2011-06-23 17:27:42]

Took some effort, but finally addressed Sergey's comment.

http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h#...
net/socket/stream_socket.h:86: virtual int GetConnectTimeMicros() const = 0;
On 2011/06/21 17:25:16, sergeyu wrote:
> Maybe use base::TimeDelta instead of int here?

Done.

[gagansingh, 2011-06-23 20:09:15]

Hi Will

win_sync<http://build.chromium.org/p/tryserver.chromium/builders/win_sync/builds/3334>,
linux_sync<http://build.chromium.org/p/tryserver.chromium/builders/linux_sync/builds/3600>,
linux<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/33783>and
mac_sync<http://build.chromium.org/p/tryserver.chromium/builders/mac_sync/builds/3282>have
passed.
win<http://build.chromium.org/p/tryserver.chromium/builders/win/builds/38986>and
mac<http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/34326>have
failed, but the errors look unrelated to this patch.

What do we do next?

Thanks
Gagan




On Thu, Jun 23, 2011 at 10:57 PM, <gagansingh@google.com> wrote:

> Took some effort, but finally addressed Sergey's comment.
>
>
>
> http://codereview.chromium.**org/7189055/diff/11009/net/**
>
socket/stream_socket.h<http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h>
> File net/socket/stream_socket.h (right):
>
> http://codereview.chromium.**org/7189055/diff/11009/net/**
>
socket/stream_socket.h#**newcode86<http://codereview.chromium.org/7189055/diff/11009/net/socket/stream_socket.h#newcode86>
> net/socket/stream_socket.h:86: virtual int GetConnectTimeMicros() const
> = 0;
> On 2011/06/21 17:25:16, sergeyu wrote:
>
>> Maybe use base::TimeDelta instead of int here?
>>
>
> Done.
>
>
http://codereview.chromium.**org/7189055/<http://codereview.chromium.org/7189...
>

[willchan no longer on Chromium, 2011-06-24 11:11:35]

Fix up these last nits and I'll send it to the commit-bot again.

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
File chrome/browser/browser_main_unittest.cc (right):

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
chrome/browser/browser_main_unittest.cc:8: #include
"chrome/browser/browser_main.h"
This header goes first.

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
chrome/browser/browser_main_unittest.cc:48: static const int kNumRuns = 1000;
Drop the static, then the compiler will just turn this into an intermediate
constant in the assembly code.

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:8: #include "base/command_line.h"
You don't need this header, do you?

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:643: EXPECT_EQ(sockets_[0],
req->handle()->socket());
Please verify that the NumBytesRead() is as you expect.

[gagansingh, 2011-06-24 12:17:56]

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
File chrome/browser/browser_main_unittest.cc (right):

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
chrome/browser/browser_main_unittest.cc:8: #include
"chrome/browser/browser_main.h"
On 2011/06/24 11:11:36, willchan wrote:
> This header goes first.

Done.

http://codereview.chromium.org/7189055/diff/2243/chrome/browser/browser_main_...
chrome/browser/browser_main_unittest.cc:48: static const int kNumRuns = 1000;
On 2011/06/24 11:11:36, willchan wrote:
> Drop the static, then the compiler will just turn this into an intermediate
> constant in the assembly code.

Done.

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base.cc (right):

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
net/socket/client_socket_pool_base.cc:8: #include "base/command_line.h"
On 2011/06/24 11:11:36, willchan wrote:
> You don't need this header, do you?

Done.

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/7189055/diff/2243/net/socket/client_socket_poo...
net/socket/client_socket_pool_base_unittest.cc:643: EXPECT_EQ(sockets_[0],
req->handle()->socket());
On 2011/06/24 11:11:36, willchan wrote:
> Please verify that the NumBytesRead() is as you expect.

Done.

[commit-bot: I haz the power, 2011-06-24 13:00:49]

Can't process patch for file net/socket/socket_test_util.cc.
File's status is None, patchset upload is incomplete.

[gagansingh, 2011-06-24 13:33:40]

Reuploaded the patch, looks okay now. Please try again.

Thanks
Gagan




On Fri, Jun 24, 2011 at 6:30 PM, <commit-bot@chromium.org> wrote:

> Can't process patch for file net/socket/socket_test_util.**cc.
>
> File's status is None, patchset upload is incomplete.
>
>
http://codereview.chromium.**org/7189055/<http://codereview.chromium.org/7189...
>

[commit-bot: I haz the power, 2011-06-24 16:47:10]

Change committed as 90373

[gagansingh, 2011-06-24 17:41:37]

Yayy :) Thanks lots for the reviews and support guys. Lets hope this gives
us good improvements in the field.

Thanks
Gagan


On Fri, Jun 24, 2011 at 10:17 PM, <commit-bot@chromium.org> wrote:

> Change committed as 90373
>
>
>
http://codereview.chromium.**org/7189055/<http://codereview.chromium.org/7189...
>