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 CONTINUED here : http://codereview.chromium.org/7189055/

[willchan no longer on Chromium, 2011-06-06 10:57:36]

For cwnd calculations, the only bytes read that matters is the one for the
underlying transport socket, the TCPClientSocketFoo objects. You should not be
recording the bytes received for the higher level sockets. That is wrong. Please
refer to how something like
http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/net/h...
is done. It just forwards the call to the underlying socket until we reach the
transport socket. That's what you should be doing.

http://codereview.chromium.org/6990036/diff/30001/net/http/http_basic_stream.h
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/6990036/diff/30001/net/http/http_basic_stream....
net/http/http_basic_stream.h:84: virtual double GetRTTMs() const OVERRIDE;
When I first read this, I was wondering what a RTTM was. Then I realized Ms is
milliseconds. It'd be clearer if you named this GetRTTInMs()

http://codereview.chromium.org/6990036/diff/30001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6990036/diff/30001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1079: int num_kb =
stream_->GetBytesReceived() / 1024;
I think the way you have this structured isn't what you want. This stats are
sort of meaningless for SPDY since we will only be using one connection. Why
don't you add a LogRTTMetrics or something to the HttpStream interface instead
of exposing accessors. Then for HttpBasicStream, do this logic. For
SpdyHttpStream, where it doesn't matter, don't do anything.

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

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:84: // Returns the number of bytes successfully
written to this socket.
This should be virtual int NumBytesRead() const = 0;

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:88: virtual double GetRTTMs() const { return rtt_ms_;
}
virtual double GetRTTInMs() const = 0;

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:138: int num_bytes_read_;
Don't add any concrete member variables into this abstract interface class.

You want to add these guys into the transport sockets (TCPClientSocketLibevent
and TCPClientSocketWin). Higher level sockets like the HttpProxyClientSocket
should just forward to their underlying sockets.

http://codereview.chromium.org/6990036/diff/30001/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/6990036/diff/30001/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:74: LOG(ERROR) << "Should not be called";
Use NOTREACHED() instead.

[wtc, 2011-06-06 17:24:05]

I only reviewed the API (.h files) changes in src/net in
Patch Set 17.

http://codereview.chromium.org/6990036/diff/31083/net/http/http_stream.h
File net/http/http_stream.h (right):

http://codereview.chromium.org/6990036/diff/31083/net/http/http_stream.h#newc...
net/http/http_stream.h:138: virtual void LogRttVsBytesMetrics() const = 0;
Please document this method.  I guess you can copy the comment
from net/http/http_network_transaction.h.

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

http://codereview.chromium.org/6990036/diff/31083/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:292: static double
set_bytes_read_vs_last_accessed_alpha(double alpha);
Please document this method.  (I don't know what "alpha" means.)

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

http://codereview.chromium.org/6990036/diff/31083/net/socket/stream_socket.h#...
net/socket/stream_socket.h:83: virtual int NumBytesRead() const = 0;
The comment says "written" but the function name says "Read".
I guess the comment is wrong?

http://codereview.chromium.org/6990036/diff/31083/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/6990036/diff/31083/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.h:201: int num_bytes_read_;
This byte count should be int64 to allow a long-lived
connection to transfer more than 2GB.

Make the same change to net/socket/tcp_client_socket_win.h.

[Gagan, 2011-06-06 20:27:09]

Hi guys

I addressed most of your comments, but i'm running into a weird problem. Have
tried this on ubuntu and mac.

I built chrome using this patch, and loaded
http://lh3.googleusercontent.com/-I6KNzVNTvSk/TVmMYUmQ7cI/AAAAAAAAALk/lIRSGl9...
Its content length header = 113285, have verified with tcpdump that indeed this
much data is being transferred.

But if i look at the bytes read returned by TCPClientSocketLibevent::Read, its
much lower than the total file size. Here is one sample log:

[12978:12991:1141810710848:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 11680
[12978:12991:1141810746507:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 11502
[12978:12991:1141810772284:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 7090
[12978:12991:1141810820090:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 4254
[12978:12991:1141810830919:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 1418
[12978:12991:1141810861506:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 8666
[12978:12991:1141810869683:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 1418
[12978:12991:1141810884096:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 8508
[12978:12991:1141810893260:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 5672
[12978:12991:1141810904708:ERROR:tcp_client_socket_libevent.cc(357)] this =
0x4f6c690   Read 4412
[12978:12991:1141810917305:ERROR:http_basic_stream.cc(138)] GET
/-I6KNzVNTvSk/TVmMYUmQ7cI/AAAAAAAAALk/lIRSGl9Nltg/s576/IMG_20110214_173612.jpg
HTTP/1.1

rtt = 7.254     num_rtt = 43.2865       num_kb = 63     val = 177215    total
bytes = 320866

It says it has read only 63KB, where it should have read 113KB.

Any ideas what im doing wrong?

Thanks
Gagan

http://codereview.chromium.org/6990036/diff/30001/net/http/http_basic_stream.h
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/6990036/diff/30001/net/http/http_basic_stream....
net/http/http_basic_stream.h:84: virtual double GetRTTMs() const OVERRIDE;
On 2011/06/06 10:57:36, willchan wrote:
> When I first read this, I was wondering what a RTTM was. Then I realized Ms is
> milliseconds. It'd be clearer if you named this GetRTTInMs()

Done.

http://codereview.chromium.org/6990036/diff/30001/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6990036/diff/30001/net/http/http_network_trans...
net/http/http_network_transaction.cc:1079: int num_kb =
stream_->GetBytesReceived() / 1024;
On 2011/06/06 10:57:36, willchan wrote:
> I think the way you have this structured isn't what you want. This stats are
> sort of meaningless for SPDY since we will only be using one connection. Why
> don't you add a LogRTTMetrics or something to the HttpStream interface instead
> of exposing accessors. Then for HttpBasicStream, do this logic. For
> SpdyHttpStream, where it doesn't matter, don't do anything.

Good point.
Done.

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

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:84: // Returns the number of bytes successfully
written to this socket.
On 2011/06/06 10:57:36, willchan wrote:
> This should be virtual int NumBytesRead() const = 0;

Done.

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:88: virtual double GetRTTMs() const { return rtt_ms_;
}
On 2011/06/06 10:57:36, willchan wrote:
> virtual double GetRTTInMs() const = 0;

Done.

http://codereview.chromium.org/6990036/diff/30001/net/socket/stream_socket.h#...
net/socket/stream_socket.h:138: int num_bytes_read_;
On 2011/06/06 10:57:36, willchan wrote:
> Don't add any concrete member variables into this abstract interface class.
> 
> You want to add these guys into the transport sockets (TCPClientSocketLibevent
> and TCPClientSocketWin). Higher level sockets like the HttpProxyClientSocket
> should just forward to their underlying sockets.

Done.

http://codereview.chromium.org/6990036/diff/30001/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/6990036/diff/30001/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:74: LOG(ERROR) << "Should not be called";
On 2011/06/06 10:57:36, willchan wrote:
> Use NOTREACHED() instead.

Replaced with LogRttVsBytesMetrics as you suggested and not doing anything in
it.

http://codereview.chromium.org/6990036/diff/31083/net/http/http_stream.h
File net/http/http_stream.h (right):

http://codereview.chromium.org/6990036/diff/31083/net/http/http_stream.h#newc...
net/http/http_stream.h:138: virtual void LogRttVsBytesMetrics() const = 0;
On 2011/06/06 17:24:05, wtc wrote:
> Please document this method.  I guess you can copy the comment
> from net/http/http_network_transaction.h.

Done.

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

http://codereview.chromium.org/6990036/diff/31083/net/socket/stream_socket.h#...
net/socket/stream_socket.h:83: virtual int NumBytesRead() const = 0;
On 2011/06/06 17:24:05, wtc wrote:
> The comment says "written" but the function name says "Read".
> I guess the comment is wrong?

Done.

[gagansingh, 2011-06-07 16:32:01]

Hi

Figured out what was going wrong:
I had missed DidCompleteRead() method which amazingly also reads from the
socket.

Now the bytes transferred on the socket agree with the data transferred on the
wire.

Please take another look, i'l clean up the logging statements if you are okay
with the direction.
Also, how do i start looking at the histograms. Are they sent to some server for
analysis? if so, can i send these to my server and try to make some sense out of
them ?

Thanks
Gagan

[Gagan, 2011-06-08 16:26:37]

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

http://codereview.chromium.org/6990036/diff/31083/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:292: static double
set_bytes_read_vs_last_accessed_alpha(double alpha);
On 2011/06/06 17:24:05, wtc wrote:
> Please document this method.  (I don't know what "alpha" means.)

renamed to tcp_socket_estimated_cwnd_decay_coef and added a comment explaining
its use.
its a coefficient used to decay the estimated cwnd of a socket based on its idle
time.

http://codereview.chromium.org/6990036/diff/31083/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.h (right):

http://codereview.chromium.org/6990036/diff/31083/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.h:201: int num_bytes_read_;
On 2011/06/06 17:24:05, wtc wrote:
> This byte count should be int64 to allow a long-lived
> connection to transfer more than 2GB.
> 
> Make the same change to net/socket/tcp_client_socket_win.h.

Done.

[willchan no longer on Chromium, 2011-06-09 15:08:54]

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:18: std::string
g_warm_connection_field_trial_group;
This is not allowed by the style guide, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static....

http://codereview.chromium.org/6990036/diff/47017/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_network_trans...
net/http/http_network_transaction.cc:1087: void
HttpNetworkTransaction::LogNumRttVsBytesMetrics() const {
Does this really need to be a function?

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

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:397: double B =
static_cast<double>(bytes_read) / 1024.0;
nit: fix your variable naming, they should follow
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab....

as well as:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

It seems like you're trying to follow some function. I am not familiar with the
function, so using abbreviations like this are just confusing. Please use full
names.

You also need way more comments to explain what the calculations you are doing
mean and why you are doing this.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:146: connect_start_time_(),
No need to explicitly invoke the empty constructor, it'll do so automatically
anyway.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:351: rtt_ms_ =
static_cast<double>((base::Time::Now() -
Why is this kept as a double? Seems better to use an int. It can be converted
where used if needed.

Also, for Linux, why are we calculating this instead of using TCP_INFO
(http://linuxgazette.net/136/pfeiffer.html)?

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:531: / 1000.0;
same comments as in the libevent socket

[Mike Belshe, 2011-06-09 16:08:51]

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

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:497: const base::FieldTrial::Probability
kWarmSocketProbability = 33;  // 1% prob.
The comment is wrong here

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:505: // This (1000000) is the current default
value.
I think the comment is wrong here too?

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:515:
ClientSocketPoolBaseHelper::set_tcp_socket_estimated_cwnd_decay_coef(
This seems like a strange place to set the policy.

I would have preferred that the ClientSocketPoolBaseHelper have:

enum {
   USE_WARMEST_SOCKET,
   USE_WARM_SOCKET,
   USE_LAST_SOCKET
} ClientSocketReusePolicy;

and then

SetSocketReusePolicy(USE_WARMEST_SOCKET)

would internally do:
   set_tcp_socket_estimated_cwnd_decay_coef(1000000)

In other words, the reader of code here in browser main shouldn't need to know
the guts of policy decisions deep inside the socket code.

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:260: extern const char
kTcpSocketEstimatedCwndDecayCoef[];
yeah - seems too deep to expose this

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:140: int val = 1024 * num_rtt_scaled + num_kb;  //
Supports ~500 RTT of 1418 B.
Can you add comments about why you're doing this math?  I understood the
computation of num_rtt, (total time / rtt), but I don't understand the reason
for multiplying num_rtt * 4096 (4 * 1024).  Finally, you've got num_kb, which is
KB, and you're ADDING that to num_rtts, so the units don't match?

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:144: HISTOGRAM_CUSTOM_COUNTS(histogram, val, 4096,
3048000, 1000);
bug:  This histogram code is incorrect, note that the HISTOGRAM_CUSTOM_COUNTS
macro creates a static, so the string for this histogram name cannot be dynamic
like this.

Comments to explain the size of this histogram would be interesting - 1000
buckets is a lot, btw.

http://codereview.chromium.org/6990036/diff/47017/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.h:82: virtual double GetRTTInMs() const;
I agree with Will - GetRTTInMs should return an int32 or int64.

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

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:402: double T =
pow(g_tcp_socket_estimated_cwnd_decay_coef, idle_time_mins);
I believe this math is broken and we have to find a different algorithm.

The units of B are bytes.
The units of T are minutes (squared).

I don't know of any way to do interesting addition/subtraction when units are
different.  This is not a rate (like B / T (bytes/sec)).  It seems like you've
found a heuristic which kinda sorta seems to work, but it's meaningless
mathematically.

I realize you're just trying to implement a damper.  Perhaps you can explain why
you think this works?

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:404: debug_log =
StringPrintf("%s\n%f\t%f\t%f",
this debug_log appears unused (or overridden below)

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:692: LOG(ERROR) << "Setting
g_tcp_socket_estimated_cwnd_decay_coef = " << coef;
nit:  this isn't a LOG(ERROR), LOG(DEBUG) or remove?

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:440: LOG(ERROR) << "this = " << (int*)
this << "\tnread = " << nread;
nit: please remove the debugging lines.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.h:59: virtual double GetRTTInMs() const {
return rtt_ms_; }
Nit:  I like that you are exposing this function off the socket, but it's not
really RTT.  It's ConnectTime().  Can we change the name of this method to
reflect that?  It's okay in other parts of the code to use ConnectTime() as an
approximation for RTT, but it is simply not true that this is the RTT.

[Gagan, 2011-06-09 19:49:25]

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

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:497: const base::FieldTrial::Probability
kWarmSocketProbability = 33;  // 1% prob.
On 2011/06/09 16:08:52, Mike Belshe wrote:
> The comment is wrong here

Done.

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:505: // This (1000000) is the current default
value.
On 2011/06/09 16:08:52, Mike Belshe wrote:
> I think the comment is wrong here too?

Done.

http://codereview.chromium.org/6990036/diff/47017/chrome/browser/browser_main...
chrome/browser/browser_main.cc:515:
ClientSocketPoolBaseHelper::set_tcp_socket_estimated_cwnd_decay_coef(
On 2011/06/09 16:08:52, Mike Belshe wrote:
> This seems like a strange place to set the policy.
> 
> I would have preferred that the ClientSocketPoolBaseHelper have:
> 
> enum {
>    USE_WARMEST_SOCKET,
>    USE_WARM_SOCKET,
>    USE_LAST_SOCKET
> } ClientSocketReusePolicy;
> 
> and then
> 
> SetSocketReusePolicy(USE_WARMEST_SOCKET)
> 
> would internally do:
>    set_tcp_socket_estimated_cwnd_decay_coef(1000000)
> 
> In other words, the reader of code here in browser main shouldn't need to know
> the guts of policy decisions deep inside the socket code.

Done.

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:260: extern const char
kTcpSocketEstimatedCwndDecayCoef[];
On 2011/06/09 16:08:52, Mike Belshe wrote:
> yeah - seems too deep to expose this

i want to be able to specify the value directly on command line for
experimentation with different values.
I want to try this out on say webpagetest.

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:18: std::string
g_warm_connection_field_trial_group;
On 2011/06/09 15:08:54, willchan wrote:
> This is not allowed by the style guide, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static....

I was following
http://codesearch.google.com/#OAMlx_jo-ck/src/net/socket/client_socket_pool_m...
as an example.
Changed to static member of class here, and kept the
client_socket_pool_manager.cc as static variable in file for file level
consistency.

If you want, in a separate small cl, i could move all static file variables of
client_socket_pool_manager.cc to static class members.

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:140: int val = 1024 * num_rtt_scaled + num_kb;  //
Supports ~500 RTT of 1418 B.
On 2011/06/09 16:08:52, Mike Belshe wrote:
> Can you add comments about why you're doing this math?  I understood the
> computation of num_rtt, (total time / rtt), but I don't understand the reason
> for multiplying num_rtt * 4096 (4 * 1024).  Finally, you've got num_kb, which
is
> KB, and you're ADDING that to num_rtts, so the units don't match?

num_rtt is a double, so scaling it by 4 to basically distinguish 3.0, 3.25, 3.5,
3.75.

The metric used in histogram is more like a 2D graph.
The only measure of success of warmest connection first over last accessed first
i could think of was more number of bytes per rtt. this is kind of independent
of users browsing pattern.
So i thought i could uniquely represent each (num_kb, rtt) pair as 1024* num_rtt
+ num_kb

From the histogram of this metric, we can compute the num_kb vs rtt graph, and
the graph of warmest first should be higher.

Sorry for writing it in a confusing way, will be happy to hear other metrics
that can measure.

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:144: HISTOGRAM_CUSTOM_COUNTS(histogram, val, 4096,
3048000, 1000);
On 2011/06/09 16:08:52, Mike Belshe wrote:
> bug:  This histogram code is incorrect, note that the HISTOGRAM_CUSTOM_COUNTS
> macro creates a static, so the string for this histogram name cannot be
dynamic
> like this.
the counter is static, but its value is the return value of
Histogram::FactoryGet() which takes the name. I don't think this is a problem.
Have verified the histogram name in about:histograms as well.

> 
> Comments to explain the size of this histogram would be interesting - 1000
> buckets is a lot, btw.

histogram buckets seem to be exponential in size. I want something with higher
granularity on the higher buckets because thats where the effect is more
visible.

http://codereview.chromium.org/6990036/diff/47017/net/http/http_network_trans...
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_network_trans...
net/http/http_network_transaction.cc:1087: void
HttpNetworkTransaction::LogNumRttVsBytesMetrics() const {
On 2011/06/09 15:08:54, willchan wrote:
> Does this really need to be a function?

removed

http://codereview.chromium.org/6990036/diff/47017/net/http/http_proxy_client_...
File net/http/http_proxy_client_socket.h (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_proxy_client_...
net/http/http_proxy_client_socket.h:82: virtual double GetRTTInMs() const;
On 2011/06/09 16:08:52, Mike Belshe wrote:
> I agree with Will - GetRTTInMs should return an int32 or int64.

changed to GetConnectTimeMicros()

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

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:397: double B =
static_cast<double>(bytes_read) / 1024.0;
On 2011/06/09 15:08:54, willchan wrote:
> nit: fix your variable naming, they should follow
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab....
> 
> as well as:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...
>
Changed the variable names to follow convention.
 
> It seems like you're trying to follow some function. I am not familiar with
the
> function, so using abbreviations like this are just confusing. Please use full
> names.
> 
> You also need way more comments to explain what the calculations you are doing
> mean and why you are doing this.

Nothing complicated, simply num_kbs - (coef ^ idle_time_in_mins)
coef = 1 prefers warmest connection
coef = 1.83 simulates cwnd decay of 20KB in 5 minutes.
This function partly simulates cwnd validation and does not penalize cwnd for
smaller idle times.

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:402: double T =
pow(g_tcp_socket_estimated_cwnd_decay_coef, idle_time_mins);
On 2011/06/09 16:08:52, Mike Belshe wrote:
> I believe this math is broken and we have to find a different algorithm.
> 
> The units of B are bytes.
> The units of T are minutes (squared).
> 
> I don't know of any way to do interesting addition/subtraction when units are
> different.  This is not a rate (like B / T (bytes/sec)).  It seems like you've
> found a heuristic which kinda sorta seems to work, but it's meaningless
> mathematically.
> 
> I realize you're just trying to implement a damper.  Perhaps you can explain
why
> you think this works?

Yes, i want a damping function.
Im choosing coef ^ idle_time because it simulates cwnd validation, reduction of
cwnd by a factor of coef every RTO. Plus, this simulates a non linear
probability increase of connection close as idle time increases.
I thought a lot about what kind of function to choose, and this one kind of felt
right. Would love to hear your ideas.

20 KB / 5 mins was chosen conservatively. I think warmest connection first is
the way to go, but a warm with smaller idle time is also good. Hence choosing
one more value. If there are many cases where missing FIN packet causes
problems, then the stat for this should be better than warmest first.

You are right that there is a mismatch in units, thats because i don't have any
estimate of bandwidth or a good rate function.

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:404: debug_log =
StringPrintf("%s\n%f\t%f\t%f",
On 2011/06/09 16:08:52, Mike Belshe wrote:
> this debug_log appears unused (or overridden below)

its appending to its previous value

http://codereview.chromium.org/6990036/diff/47017/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:692: LOG(ERROR) << "Setting
g_tcp_socket_estimated_cwnd_decay_coef = " << coef;
On 2011/06/09 16:08:52, Mike Belshe wrote:
> nit:  this isn't a LOG(ERROR), LOG(DEBUG) or remove?

Changed to LOG(DEBUG)

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:146: connect_start_time_(),
On 2011/06/09 15:08:54, willchan wrote:
> No need to explicitly invoke the empty constructor, it'll do so automatically
> anyway.

Done.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:351: rtt_ms_ =
static_cast<double>((base::Time::Now() -
On 2011/06/09 15:08:54, willchan wrote:
> Why is this kept as a double? Seems better to use an int. It can be converted
> where used if needed.
> 
> Also, for Linux, why are we calculating this instead of using TCP_INFO
> (http://linuxgazette.net/136/pfeiffer.html%29?

Initially i chose double because i didn't want to add special handling for rtt's
< 1 ms
Besides, i think rtt is usually measured at higher granularity than integer
number of milliseconds. Ping shows it to 3 decimal places.
If you want i could change the function to GetRttInMicros().

Macosx also seems to be using this file., and i couldn't find an easy of getting
rtt. I think tcp_info is linux specific,  on mac /usr/include/netinet/tcp.h
doesnt have TCP_INFO.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:440: LOG(ERROR) << "this = " << (int*)
this << "\tnread = " << nread;
On 2011/06/09 16:08:52, Mike Belshe wrote:
> nit: please remove the debugging lines.

Will remove in the last round.
For now, please accomodate the few extra lines. They are extremely helpful in
tracking sockets when something goes wrong.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:531: / 1000.0;
On 2011/06/09 15:08:54, willchan wrote:
> same comments as in the libevent socket

please let me know if you prefer rtt in microseconds.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.h (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.h:59: virtual double GetRTTInMs() const {
return rtt_ms_; }
On 2011/06/09 16:08:52, Mike Belshe wrote:
> Nit:  I like that you are exposing this function off the socket, but it's not
> really RTT.  It's ConnectTime().  Can we change the name of this method to
> reflect that?  It's okay in other parts of the code to use ConnectTime() as an
> approximation for RTT, but it is simply not true that this is the RTT.

Done.

[willchan no longer on Chromium, 2011-06-10 15:05:55]

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:18: std::string
g_warm_connection_field_trial_group;
On 2011/06/09 19:49:26, gagan.goku wrote:
> On 2011/06/09 15:08:54, willchan wrote:
> > This is not allowed by the style guide, see
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static....
> 
> I was following
>
http://codesearch.google.com/#OAMlx_jo-ck/src/net/socket/client_socket_pool_m...
> as an example.
> Changed to static member of class here, and kept the
> client_socket_pool_manager.cc as static variable in file for file level
> consistency.
> 
> If you want, in a separate small cl, i could move all static file variables of
> client_socket_pool_manager.cc to static class members.

You misunderstand. It doesn't matter if it's a file local static or a class
static. They're both globals. They create static initializers and destructors
that lead to global construction/destruction ordering bugs/crashes and also slow
down startup/shutdown. The style guide prohibits global non-POD.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:531: / 1000.0;
On 2011/06/09 19:49:26, gagan.goku wrote:
> On 2011/06/09 15:08:54, willchan wrote:
> > same comments as in the libevent socket
> 
> please let me know if you prefer rtt in microseconds.

milliseconds makes sense to me.

[Gagan, 2011-06-10 17:44:16]

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/http/http_basic_stream....
net/http/http_basic_stream.cc:18: std::string
g_warm_connection_field_trial_group;
On 2011/06/10 15:05:55, willchan wrote:
> On 2011/06/09 19:49:26, gagan.goku wrote:
> > On 2011/06/09 15:08:54, willchan wrote:
> > > This is not allowed by the style guide, see
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static....
> > 
> > I was following
> >
>
http://codesearch.google.com/#OAMlx_jo-ck/src/net/socket/client_socket_pool_m...
> > as an example.
> > Changed to static member of class here, and kept the
> > client_socket_pool_manager.cc as static variable in file for file level
> > consistency.
> > 
> > If you want, in a separate small cl, i could move all static file variables
of
> > client_socket_pool_manager.cc to static class members.
> 
> You misunderstand. It doesn't matter if it's a file local static or a class
> static. They're both globals. They create static initializers and destructors
> that lead to global construction/destruction ordering bugs/crashes and also
slow
> down startup/shutdown. The style guide prohibits global non-POD.

Ah sorry, missed it completely. Changed to global int.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:531: / 1000.0;
On 2011/06/10 15:05:55, willchan wrote:
> On 2011/06/09 19:49:26, gagan.goku wrote:
> > On 2011/06/09 15:08:54, willchan wrote:
> > > same comments as in the libevent socket
> > 
> > please let me know if you prefer rtt in microseconds.
> 
> milliseconds makes sense to me.

I changed it to micros in the last patch. It allows extra precision so that
round off errors will not be prevalent when rtt is low.
I hope you are okay with it, please let me know if you strongly feel against it.

[Mike Belshe, 2011-06-10 18:11:42]

Thanks for taking the time to go through all these comments.  I realize we are
very fussy in our code reviews!

Mike

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:260: extern const char
kTcpSocketEstimatedCwndDecayCoef[];
That seems wrong to me, you're violating common encapsulation - you've put the
policy into the browser_main, and code in the net layer.

Do you really need to test arbitrary values on webpagetest?  Those will be
anecdotal results anyway.  Picking some specific damping coefficients and then
using the A/B test that you've got is superior.


On 2011/06/09 19:49:26, gagan.goku wrote:
> On 2011/06/09 16:08:52, Mike Belshe wrote:
> > yeah - seems too deep to expose this
> 
> i want to be able to specify the value directly on command line for
> experimentation with different values.
> I want to try this out on say webpagetest.

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream....
net/http/http_basic_stream.cc:141: int val = 1024 * num_rtt_scaled + num_kb;
I think this is ridiculous still.  I see that you're generating a num_kb which
is 0 to 1024, and combining that with a num_rtt which has been multiplied to
offset it within the value.

But what is the point of such a histogram?  Histograms go into exponential
buckets, and this is just two independent values mixed - you'll end up with a
really whacky plot.  You won't have meaningful data here, because histogram
buckets are not linear.

You'll not like this, but you can collect multiple buckets of RTTs.  So you
could do num RTTs for 0-10KB, number of RTTs for 10-20KB, number of RTTS for
20-30KB, etc.

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream....
net/http/http_basic_stream.cc:144:
histogram.append(g_warm_connection_field_trial_group_);
OK - I see, g_warm_connection_field_trial_group is constant, so the histogram
string will always be the same for any single instance of the browser.

I think the code you're looking for is this:

HISTOGRAM_CUSTOM_COUNTS(FieldTrial::MakeName(std::string("Net.RTT_vs_KB"),
g_warm_connection_field_trial_group_), <etc>

BTW -

If you want us to be able to see the numbers from the field you need to use

UMA_HISTOGRAM_CUSTOM_COUNTS instead of HISTOGRAM_CUSTOM_COUNTS

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

http://codereview.chromium.org/6990036/diff/47040/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:395: double benefit = 0;
My thoughts on this function:

* You're calculating a score for each socket based on number of bytes sent and
dampened by idle time.

* Idle times measured in minutes probably aren't that useful, since we tend to
timeout sockets faster than that.

* Damping functions usually damp exponentially (you have this already)

Current score is:
  score = num_kb_sent - (coef ^ idle_time_in_mins)

Note:  scores are unbounded here and eventually go negative.  This function only
appears to work because time was converted to minutes.  Try using seconds, and
the damping goes out of whack very quickly.

Proposal:
  idle_time = min(1, idle_time_in_sec);
  score = num_kb_sent / (idle_time_in_sec ^ coef)
  coef == 0.25 (my guess)

  Note:  the minimum score here is 0, and the damping works fine with seconds,
giving us damping across reasonable units (seconds), and it scales up to any
value.

I'll send you a spreadsheet comparing these two separately.

[Gagan, 2011-06-12 20:11:44]

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/47017/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:260: extern const char
kTcpSocketEstimatedCwndDecayCoef[];
On 2011/06/10 18:11:42, Mike Belshe wrote:
> That seems wrong to me, you're violating common encapsulation - you've put the
> policy into the browser_main, and code in the net layer.
> 
> Do you really need to test arbitrary values on webpagetest?  Those will be
> anecdotal results anyway.  Picking some specific damping coefficients and then
> using the A/B test that you've got is superior.
> 
> 
> On 2011/06/09 19:49:26, gagan.goku wrote:
> > On 2011/06/09 16:08:52, Mike Belshe wrote:
> > > yeah - seems too deep to expose this
> > 
> > i want to be able to specify the value directly on command line for
> > experimentation with different values.
> > I want to try this out on say webpagetest.
> 

Done.

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream....
net/http/http_basic_stream.cc:141: int val = 1024 * num_rtt_scaled + num_kb;
On 2011/06/10 18:11:42, Mike Belshe wrote:
> I think this is ridiculous still.  I see that you're generating a num_kb which
> is 0 to 1024, and combining that with a num_rtt which has been multiplied to
> offset it within the value.
> 
> But what is the point of such a histogram?  Histograms go into exponential
> buckets, and this is just two independent values mixed - you'll end up with a
> really whacky plot.  You won't have meaningful data here, because histogram
> buckets are not linear.
> 
> You'll not like this, but you can collect multiple buckets of RTTs.  So you
> could do num RTTs for 0-10KB, number of RTTs for 10-20KB, number of RTTS for
> 20-30KB, etc.

Done.

http://codereview.chromium.org/6990036/diff/47040/net/http/http_basic_stream....
net/http/http_basic_stream.cc:144:
histogram.append(g_warm_connection_field_trial_group_);
On 2011/06/10 18:11:42, Mike Belshe wrote:
> OK - I see, g_warm_connection_field_trial_group is constant, so the histogram
> string will always be the same for any single instance of the browser.
> 
> I think the code you're looking for is this:
> 
> HISTOGRAM_CUSTOM_COUNTS(FieldTrial::MakeName(std::string("Net.RTT_vs_KB"),
> g_warm_connection_field_trial_group_), <etc>
> 
> BTW -
> 
> If you want us to be able to see the numbers from the field you need to use
> 
> UMA_HISTOGRAM_CUSTOM_COUNTS instead of HISTOGRAM_CUSTOM_COUNTS

Done.

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

http://codereview.chromium.org/6990036/diff/47040/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:395: double benefit = 0;
On 2011/06/10 18:11:42, Mike Belshe wrote:
> My thoughts on this function:
> 
> * You're calculating a score for each socket based on number of bytes sent and
> dampened by idle time.
> 
> * Idle times measured in minutes probably aren't that useful, since we tend to
> timeout sockets faster than that.
> 
> * Damping functions usually damp exponentially (you have this already)
> 
> Current score is:
>   score = num_kb_sent - (coef ^ idle_time_in_mins)
> 
> Note:  scores are unbounded here and eventually go negative.  This function
only
> appears to work because time was converted to minutes.  Try using seconds, and
> the damping goes out of whack very quickly.
> 
> Proposal:
>   idle_time = min(1, idle_time_in_sec);
>   score = num_kb_sent / (idle_time_in_sec ^ coef)
>   coef == 0.25 (my guess)
> 
>   Note:  the minimum score here is 0, and the damping works fine with seconds,
> giving us damping across reasonable units (seconds), and it scales up to any
> value.
> 
> I'll send you a spreadsheet comparing these two separately.

This function penalizes num_kb lesser than the older function, plus looks nice,
so using it.
Thanks for the suggestion.

[Mike Belshe, 2011-06-12 22:10:35]

Pretty close.  If Will is ok with this, then lgtm.

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

http://codereview.chromium.org/6990036/diff/48177/chrome/browser/browser_main...
chrome/browser/browser_main.cc:511: "warm_socket", kWarmSocketProbability);
nit:  you're now assuming that the field trial returns ints 0, 1, and 2.  ok.

http://codereview.chromium.org/6990036/diff/48177/chrome/browser/browser_main...
chrome/browser/browser_main.cc:533: NOTREACHED() << "Not a valid socket reuse
policy group";
nit:  the notreached() will crash even a release build, right?  so these two are
redundant?

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:137: int64 num_kb = bytes_received / 1024;
Note:  for integers, division  truncates.  So for 0-1023 bytes, this will be 0.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:149: int bucket = (num_kb / 5) * 5;
Again, integer math.  For 0 to 4KB this will be 0.  This is fine...

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:153: Histogram* counter = Histogram::FactoryGet(
Note, this is going to generate 600 different histograms :-)

(200 buckets and 3 combinations)

Also, you might want to use the FieldTrial::MakeName call.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:158: LOG(ERROR) << request_line_
please remove this debugging error log.

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:406: debug_log =
StringPrintf("%s\n%f\t%d\t%f", debug_log.data(),
please remove the debug logging

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:422: debug_log.data(),
please remove the debug logging.

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:301: static double SetSocketReusePolicy(int
policy);
Instead of an int, you could use a ClientSocketReusePolicy.

[Mike Belshe, 2011-06-12 22:17:03]

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (left):

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:595: std::list<IdleSocket>::iterator j =
group->mutable_idle_sockets()->begin();
BTW - do we need to clean out the socket with the fewest bytes sent on it? 
Otherwise, your algorithm might not work as well as expected.

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:412: idle_socket_it = it;
Note:  This is basically a LIFO.  The old algorithm was FIFO.  Do you want to be
able to compare:

  LIFO/FIFO/warmest/warmest-scored?

Sorry to keep adding more to your change.

[Gagan, 2011-06-13 03:52:47]

Thanks for the review Mike.

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

http://codereview.chromium.org/6990036/diff/48177/chrome/browser/browser_main...
chrome/browser/browser_main.cc:511: "warm_socket", kWarmSocketProbability);
On 2011/06/12 22:10:35, Mike Belshe wrote:
> nit:  you're now assuming that the field trial returns ints 0, 1, and 2.  ok.

Actually no.
Im assuming that all 3 groups are represented by different numbers. The array
below will find the chosen experiment group in these numbers and use the match
index.
So the array constructed here is
a[0] = warmest socket
a[1] = warm
a[2] = last accessed
which is in sync with the enum.

http://codereview.chromium.org/6990036/diff/48177/chrome/browser/browser_main...
chrome/browser/browser_main.cc:533: NOTREACHED() << "Not a valid socket reuse
policy group";
On 2011/06/12 22:10:35, Mike Belshe wrote:
> nit:  the notreached() will crash even a release build, right?  so these two
are
> redundant?

base/logging.h defines NOTREACHED as DCHECK(false)
I added the log(error) temporarily because i was playing with release builds and
wanted to see the message.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:137: int64 num_kb = bytes_received / 1024;
On 2011/06/12 22:10:35, Mike Belshe wrote:
> Note:  for integers, division  truncates.  So for 0-1023 bytes, this will be
0.

noted. 0 to 5120 bytes should all go in one bucket.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:149: int bucket = (num_kb / 5) * 5;
On 2011/06/12 22:10:35, Mike Belshe wrote:
> Again, integer math.  For 0 to 4KB this will be 0.  This is fine...

+1.
For 0 - 5kb, in normal conditions, it should take 1 or 2 rtt's. 2 bucket
histogram should be able to record both.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:153: Histogram* counter = Histogram::FactoryGet(
On 2011/06/12 22:10:35, Mike Belshe wrote:
> Note, this is going to generate 600 different histograms :-)
> 
> (200 buckets and 3 combinations)
Agreed, hopefully in a binary it wont create all 200 depending on users browsing
pattern due to lazy creation.

> 
> Also, you might want to use the FieldTrial::MakeName call.
I tried using it, but this was easier. I anyways had to append the bucket id.
Plus, to use FieldTrial::MakeName, i need the chosen group. I am passing the
enum value around, and not the chosen field trial group.

I can use FieldTrial::MakeName directly assuming that field trial groups are
also 0, 1, 2 ... in order of creation.

http://codereview.chromium.org/6990036/diff/48177/net/http/http_basic_stream....
net/http/http_basic_stream.cc:158: LOG(ERROR) << request_line_
On 2011/06/12 22:10:35, Mike Belshe wrote:
> please remove this debugging error log.

Will do so in the next round :) Please bear with me for one more round.
Logging is soo much better than debugging and tcpdump verification, though will
do so to be sure in the last round.

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
File net/socket/client_socket_pool_base.cc (left):

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:595: std::list<IdleSocket>::iterator j =
group->mutable_idle_sockets()->begin();
On 2011/06/12 22:17:03, Mike Belshe wrote:
> BTW - do we need to clean out the socket with the fewest bytes sent on it? 
> Otherwise, your algorithm might not work as well as expected.

Didn't get you. the timeout for cleaning up socket is anyways 5 minutes, so if a
socket is unused for 5 mins, its a candidate for cleanup.
Are you suggesting a higher timeout for warmest socket ?

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:412: idle_socket_it = it;
On 2011/06/12 22:17:03, Mike Belshe wrote:
> Note:  This is basically a LIFO.  The old algorithm was FIFO.  Do you want to
be
> able to compare:
> 
>   LIFO/FIFO/warmest/warmest-scored?
>
Agreed, this behaves like LIFO when response sizes are similar.
We have a field trial for FIFO, warmest and warmest scored. And i don't think
FIFO adds any value, we choose it because it simulates warmest first. So i think
we are covered for now. We should revisit this once we have some data.

Hopefully over a period of time we should be able to see meaningful data. In any
case, i can use the command line flag to use warmest first.
On blink.boutiques.com, warmest first produces awesome results, and i'm sure
there will be other sites as well where this will be useful, though may not be
so easy to measure as its on blink.boutiques.com (try it, from remote countries
like india you see a huge cut down in variability and latency).
 
> Sorry to keep adding more to your change.
np.

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:301: static double SetSocketReusePolicy(int
policy);
On 2011/06/12 22:10:35, Mike Belshe wrote:
> Instead of an int, you could use a ClientSocketReusePolicy.

was doing that initially, but ran into problems in browser_main.cc where i had
to convert the command line value to enum. So used int instead.

Have added a comment saying that policy should be a valid enum.

[willchan no longer on Chromium, 2011-06-13 15:06:07]

More nits, I need to look at how the algorithm worked out to make sure it's
kosher.

Btw, I need to look at your histogramming code some more, but I'm worried that
measuring RTTs isn't so useful. Hanging GETs may wreak your metrics.

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

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:211: using
net::internal::ClientSocketPoolBaseHelper;
internal is a net/ internal implementation detail. It should not be accessed
from outside of net/. It looks like we regressed on this before. Please don't
add another regression. Probably what you should do is make these functions be
free functions in the net namespace, although it can be in the
client_socket_pool_base.h header file. They should then set file local variables
in client_socket_pool_base.cc.

And I don't think there are enough use cases here to justify using a using
declaration. Use the fully qualified names please.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:492: LOG(ERROR) << "Chose socket_reuse_policy
coef = " << coef;
Remove this.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:493: ApplySocketReuseGroup(coef, socket_policy,
arraysize(socket_policy));
I have no idea what "coef" means. You call this with
"warmest_socket_trial_group" below. I don't see the connection between "coef"
and "warmest_socket_trial_group". Please refactor/rename your code so it makes
sense.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:521: void
BrowserMainParts::ApplySocketReuseGroup(int warmest_socket_trial_group,
Please move this into an anonymous namespace rather than making it a member
function.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:532: LOG(ERROR) << "Not a valid socket reuse
policy group";
This is redundant, just use the NOTREACHED.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main.h
File chrome/browser/browser_main.h (right):

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.h:139: void ApplySocketReuseGroup(int
warmest_socket_trial_group,
Please remove from the .h file.

http://codereview.chromium.org/6990036/diff/46113/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/46113/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:254: extern const char kSocketReusePolicy[];
Please sort this alphabetically.

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:17: using base::Histogram;
Chromium code does not often use using declarations. Stop doing this unless the
name is painfully long. Please fix all other instances.

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:147: const char* groups[] = {
"last_accessed_socket", "warmest_socket",
static const char* const kGroups[] = ...

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:158: LOG(ERROR) << request_line_
You're close to done now, please kill off all your unnecessary LOG statements.

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:438: LOG(ERROR) << "this = " << (int*)
this << "\tnread = " << nread;
Ditch these LOGs please.

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:492: connect_start_time_ =
base::Time::Now();
Use TimeTicks, not Time. Otherwise if the time zone changes or stuff like that,
this will be wrong.

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:72: // Do nothing.
Remove this comment and move it above. It should be grouped with the other
HttpStream methods.

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:142: // Not implemented yet.
Are you going to implement this?

http://codereview.chromium.org/6990036/diff/46113/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/6990036/diff/46113/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:312: NOTIMPLEMENTED();
Please do not add NOTIMPLEMENTED(). Either implement it or use NOTREACHED() if
it shouldn't be called.

[Gagan, 2011-06-13 16:45:58]

>More nits, I need to look at how the algorithm worked out to make sure it's
>kosher.
>
>Btw, I need to look at your histogramming code some more, but I'm worried that
>measuring RTTs isn't so useful. Hanging GETs may wreak your metrics.

We can put an upper limit on num rtt, or choose only samples where it num_kb is
atleast 1.4 * num_rtt, or something like that.
Lets try this out first, tweaking / adding removing more info will be easier
then.

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

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:211: using
net::internal::ClientSocketPoolBaseHelper;
On 2011/06/13 15:06:07, willchan wrote:
> internal is a net/ internal implementation detail. It should not be accessed
> from outside of net/. It looks like we regressed on this before. Please don't
> add another regression. Probably what you should do is make these functions be
> free functions in the net namespace, although it can be in the
> client_socket_pool_base.h header file. They should then set file local
variables
> in client_socket_pool_base.cc.
> 
> And I don't think there are enough use cases here to justify using a using
> declaration. Use the fully qualified names please.

Done.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:493: ApplySocketReuseGroup(coef, socket_policy,
arraysize(socket_policy));
On 2011/06/13 15:06:07, willchan wrote:
> I have no idea what "coef" means. You call this with
> "warmest_socket_trial_group" below. I don't see the connection between "coef"
> and "warmest_socket_trial_group". Please refactor/rename your code so it makes
> sense.

Done.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:521: void
BrowserMainParts::ApplySocketReuseGroup(int warmest_socket_trial_group,
On 2011/06/13 15:06:07, willchan wrote:
> Please move this into an anonymous namespace rather than making it a member
> function.

Done.

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main.h
File chrome/browser/browser_main.h (right):

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.h:139: void ApplySocketReuseGroup(int
warmest_socket_trial_group,
On 2011/06/13 15:06:07, willchan wrote:
> Please remove from the .h file.

Done.

http://codereview.chromium.org/6990036/diff/46113/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/6990036/diff/46113/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:254: extern const char kSocketReusePolicy[];
On 2011/06/13 15:06:07, willchan wrote:
> Please sort this alphabetically.

Done.

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:17: using base::Histogram;
On 2011/06/13 15:06:07, willchan wrote:
> Chromium code does not often use using declarations. Stop doing this unless
the
> name is painfully long. Please fix all other instances.

Done.

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:147: const char* groups[] = {
"last_accessed_socket", "warmest_socket",
On 2011/06/13 15:06:07, willchan wrote:
> static const char* const kGroups[] = ...

Done.

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_win.cc:492: connect_start_time_ =
base::Time::Now();
On 2011/06/13 15:06:07, willchan wrote:
> Use TimeTicks, not Time. Otherwise if the time zone changes or stuff like
that,
> this will be wrong.

Done.

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:72: // Do nothing.
On 2011/06/13 15:06:07, willchan wrote:
> Remove this comment and move it above. It should be grouped with the other
> HttpStream methods.

Done.

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:142: // Not implemented yet.
On 2011/06/13 15:06:07, willchan wrote:
> Are you going to implement this?

dont think we need to.  am not calling NOTREACHED because i think these might
get called.

http://codereview.chromium.org/6990036/diff/46113/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/6990036/diff/46113/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:312: NOTIMPLEMENTED();
On 2011/06/13 15:06:07, willchan wrote:
> Please do not add NOTIMPLEMENTED(). Either implement it or use NOTREACHED() if
> it shouldn't be called.

Done.

[Gagan, 2011-06-13 16:46:54]

>More nits, I need to look at how the algorithm worked out to make sure it's
>kosher.
>
>Btw, I need to look at your histogramming code some more, but I'm worried that
>measuring RTTs isn't so useful. Hanging GETs may wreak your metrics.

We can put an upper limit on num rtt, or choose only samples where it num_kb is
atleast 1.4 * num_rtt, or something like that.
Lets try this out first, tweaking / adding removing more info will be easier
then.

Addressed all comments except logging related, am removing these, urging you to
review the algo please.

[willchan no longer on Chromium, 2011-06-14 12:01:22]

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

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:218:
net::HttpBasicStream::SetSocketReusePolicy(i);
How about you don't call this one and instead add a net::GetSocketReusePolicy()
accessor that you call from HttpBasicStream?

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:223: LOG(ERROR) << "Not a valid socket reuse
policy group";
Either kill the LOG(ERROR) or use LOG(DFATAL) and kill the NOTREACHED.

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:506: LOG(ERROR) << "Setting socket_reuse_policy =
" << policy;
Please get rid of this LOG.

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:530: last_accessed_socket };
indentation is off, you need one more horizontal whitespace.

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:532: arraysize(policy_list));
alignment is off

http://codereview.chromium.org/6990036/diff/48195/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/48195/net/http/http_basic_stream....
net/http/http_basic_stream.cc:145: static const char* kGroups[] = {
"last_accessed_socket", "warmest_socket",
You didn't fix this.

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

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:41: double
g_socket_reuse_policy_penalty_coef = -1;
Why is this called a coefficient? It looks like an exponent.

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:52: policy ==
ClientSocketPoolBaseHelper::USE_WARMEST_SOCKET ? 0 :
Chaining a ternary operator is not very readable. Can you use if statements or a
switch statement?

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

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:56: double SetSocketReusePolicy(int
policy);
This looks really weird. What does the return value do?

You also need to use NET_API since you're calling this from outside the net/
module. Otherwise the multi-dll build will break.

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:175: enum ClientSocketReusePolicy {
It's not clear what the difference is between WARMEST and WARM socket. You need
comments.

http://codereview.chromium.org/6990036/diff/48195/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/6990036/diff/48195/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:312: LOG(ERROR) << "Should not be
called";
Don't do LOG(ERROR) AND NOTREACHED(). If you want the LOG(ERROR) in release
mode, use LOG(DFATAL).

[Gagan, 2011-06-14 18:25:02]

Removed all logging statements.

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/47017/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:440: LOG(ERROR) << "this = " << (int*)
this << "\tnread = " << nread;
On 2011/06/09 16:08:52, Mike Belshe wrote:
> nit: please remove the debugging lines.

Removed.

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

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:406: debug_log =
StringPrintf("%s\n%f\t%d\t%f", debug_log.data(),
On 2011/06/12 22:10:35, Mike Belshe wrote:
> please remove the debug logging

Done.

http://codereview.chromium.org/6990036/diff/48177/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:422: debug_log.data(),
On 2011/06/12 22:10:35, Mike Belshe wrote:
> please remove the debug logging.

Done.

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

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:492: LOG(ERROR) << "Chose socket_reuse_policy
coef = " << coef;
On 2011/06/13 15:06:07, willchan wrote:
> Remove this.

Convered to DLOG(INFO)

http://codereview.chromium.org/6990036/diff/46113/chrome/browser/browser_main...
chrome/browser/browser_main.cc:532: LOG(ERROR) << "Not a valid socket reuse
policy group";
On 2011/06/13 15:06:07, willchan wrote:
> This is redundant, just use the NOTREACHED.

Done.

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/http/http_basic_stream....
net/http/http_basic_stream.cc:158: LOG(ERROR) << request_line_
On 2011/06/13 15:06:07, willchan wrote:
> You're close to done now, please kill off all your unnecessary LOG statements.

This log is very useful as it logs most of the interesting stuff.
Put behind a VLOG_IS_ON

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/6990036/diff/46113/net/socket/tcp_client_socke...
net/socket/tcp_client_socket_libevent.cc:438: LOG(ERROR) << "this = " << (int*)
this << "\tnread = " << nread;
On 2011/06/13 15:06:07, willchan wrote:
> Ditch these LOGs please.

Done.

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

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:218:
net::HttpBasicStream::SetSocketReusePolicy(i);
On 2011/06/14 12:01:22, willchan wrote:
> How about you don't call this one and instead add a
net::GetSocketReusePolicy()
> accessor that you call from HttpBasicStream?

Done.

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:223: LOG(ERROR) << "Not a valid socket reuse
policy group";
On 2011/06/14 12:01:22, willchan wrote:
> Either kill the LOG(ERROR) or use LOG(DFATAL) and kill the NOTREACHED.

Retained only NOTREACHED

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:506: LOG(ERROR) << "Setting socket_reuse_policy =
" << policy;
On 2011/06/14 12:01:22, willchan wrote:
> Please get rid of this LOG.

Changed to DLOG(INFO)

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:530: last_accessed_socket };
On 2011/06/14 12:01:22, willchan wrote:
> indentation is off, you need one more horizontal whitespace.

Done.

http://codereview.chromium.org/6990036/diff/48195/chrome/browser/browser_main...
chrome/browser/browser_main.cc:532: arraysize(policy_list));
On 2011/06/14 12:01:22, willchan wrote:
> alignment is off

Done.

http://codereview.chromium.org/6990036/diff/48195/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/48195/net/http/http_basic_stream....
net/http/http_basic_stream.cc:145: static const char* kGroups[] = {
"last_accessed_socket", "warmest_socket",
On 2011/06/14 12:01:22, willchan wrote:
> You didn't fix this.

added the extra const u wanted

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

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:41: double
g_socket_reuse_policy_penalty_coef = -1;
On 2011/06/14 12:01:22, willchan wrote:
> Why is this called a coefficient? It looks like an exponent.

Done.

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:52: policy ==
ClientSocketPoolBaseHelper::USE_WARMEST_SOCKET ? 0 :
On 2011/06/14 12:01:22, willchan wrote:
> Chaining a ternary operator is not very readable. Can you use if statements or
a
> switch statement?

Done.

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

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:56: double SetSocketReusePolicy(int
policy);
On 2011/06/14 12:01:22, willchan wrote:
> This looks really weird. What does the return value do?
> 
> You also need to use NET_API since you're calling this from outside the net/
> module. Otherwise the multi-dll build will break.

Done.

http://codereview.chromium.org/6990036/diff/48195/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:175: enum ClientSocketReusePolicy {
On 2011/06/14 12:01:22, willchan wrote:
> It's not clear what the difference is between WARMEST and WARM socket. You
need
> comments.

Done.

http://codereview.chromium.org/6990036/diff/48195/remoting/jingle_glue/ssl_so...
File remoting/jingle_glue/ssl_socket_adapter.cc (right):

http://codereview.chromium.org/6990036/diff/48195/remoting/jingle_glue/ssl_so...
remoting/jingle_glue/ssl_socket_adapter.cc:312: LOG(ERROR) << "Should not be
called";
On 2011/06/14 12:01:22, willchan wrote:
> Don't do LOG(ERROR) AND NOTREACHED(). If you want the LOG(ERROR) in release
> mode, use LOG(DFATAL).

im going to remove the log(error)

[willchan no longer on Chromium, 2011-06-16 09:52:43]

Last nits, otherwise it LGTM.

http://codereview.chromium.org/6990036/diff/67103/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/67103/net/http/http_basic_stream....
net/http/http_basic_stream.cc:160: if (VLOG_IS_ON(2)) {
Why aren't you just doing VLOG(2) <<

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

http://codereview.chromium.org/6990036/diff/67103/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:411: int64 bytes_read =
it->socket->NumBytesRead();
Should we add something to make sure this code doesn't get called if bytes_read
is < 0? An assertion? Or perhaps just bail out of your algorithm and do LIFO?

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

http://codereview.chromium.org/6990036/diff/67103/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:54: NET_API int GetSocketReusePolicy();
Is this called outside net/? If not, we don't need NET_API.

[Gagan, 2011-06-16 11:49:29]

http://codereview.chromium.org/6990036/diff/67103/net/http/http_basic_stream.cc
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/6990036/diff/67103/net/http/http_basic_stream....
net/http/http_basic_stream.cc:160: if (VLOG_IS_ON(2)) {
On 2011/06/16 09:52:44, willchan wrote:
> Why aren't you just doing VLOG(2) <<

I thought this would be more efficient since it didn't have to do the
StringPrintf operation.
Changed to VLOG(2)

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

http://codereview.chromium.org/6990036/diff/67103/net/socket/client_socket_po...
net/socket/client_socket_pool_base.cc:411: int64 bytes_read =
it->socket->NumBytesRead();
On 2011/06/16 09:52:44, willchan wrote:
> Should we add something to make sure this code doesn't get called if
bytes_read
> is < 0? An assertion? Or perhaps just bail out of your algorithm and do LIFO?

Added a DCHECK and checking before applying algo

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

http://codereview.chromium.org/6990036/diff/67103/net/socket/client_socket_po...
net/socket/client_socket_pool_base.h:54: NET_API int GetSocketReusePolicy();
On 2011/06/16 09:52:44, willchan wrote:
> Is this called outside net/? If not, we don't need NET_API.

Was getting used only in http_stream. Removed NET_API

[commit-bot: I haz the power, 2011-06-18 11:59:03]

Presubmit check for 6990036-79005 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 Warnings **
gagan.goku@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

Presubmit checks took 1.9s to calculate.

[willchan no longer on Chromium, 2011-06-18 12:30:13]

+hclam,+ajwong for jingle/OWNERS and remoting/OWNERS.

Hey guys, need approval for the small socket changes to jingle/ and remoting/.
Cheers.

[Alpha Left Google, 2011-06-21 19:06:51]

On 2011/06/18 12:30:13, willchan wrote:
> +hclam,+ajwong for jingle/OWNERS and remoting/OWNERS.
> 
> Hey guys, need approval for the small socket changes to jingle/ and remoting/.
> Cheers.

LGTM.

[Gagan, 2011-06-22 08:41:58]

Hi Alpha

The codereview has been continued here :
http://codereview.chromium.org/7189055/ (because of CLA and other problems).
Requesting you to lgtm there.

Thanks
Gagan

On Wed, Jun 22, 2011 at 12:36 AM, <hclam@chromium.org> wrote:

> On 2011/06/18 12:30:13, willchan wrote:
>
>> +hclam,+ajwong for jingle/OWNERS and remoting/OWNERS.
>>
>
>  Hey guys, need approval for the small socket changes to jingle/ and
>> remoting/.
>> Cheers.
>>
>
> LGTM.
>
>
>
http://codereview.chromium.**org/6990036/<http://codereview.chromium.org/6990...
>