Add a histogram to measure the number of idle sockets when a TCP connection is established.

Add a histogram to measure the number of idle sockets when a TCP connection is established.

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

[willchan no longer on Chromium, 2009-04-29 21:35:43]

jar: Am I using the right histogram call?  There really ought to only be
kMaxSocketsPerGroup buckets.  6 or whatever.  Should I not be using the macros
and just manually specifying anything?

[wtc, 2009-04-29 22:48:50]

LGTM.  Some nits below.  Please wait for jar to review
the change to HttpNetworkTransaction::LogTCPConnectedMetrics.

http://codereview.chromium.org/99205/diff/1/3
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/99205/diff/1/3#newcode128
Line 128: DCHECK(i != group_map_.end());
Perhaps we should return 0 if i == group_map_.end().

The condition asserted by this DCHECK seems unnecessary
(i.e., the correctness of the code may not require this
condition), but I haven't looked at this code for a while...

http://codereview.chromium.org/99205/diff/1/4
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/99205/diff/1/4#newcode76
Line 76: int NumIdleSocketsInGroup(const std::string group_name) const;
Please add a comment.

The group_name parameter should be a const reference.

To be consistent with the idle_socket_count method above,
we should rename this method IdleSocketCountInGroup.

http://codereview.chromium.org/99205/diff/1/5
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/99205/diff/1/5#newcode1037
Line 1037: UMA_HISTOGRAM_COUNTS_100(
Please add a comment to explain why we're histogramming
this number.

[wtc, 2009-04-29 22:51:50]

http://codereview.chromium.org/99205/diff/1/5
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/99205/diff/1/5#newcode1037
Line 1037: UMA_HISTOGRAM_COUNTS_100(
On second thought, I think we shouldn't be histogramming
the number of idle sockets in group.  Instead, we should
be histogramming the boolean
  session_->connection_pool()->NumIdleSocketsInGroup(...) != 0
because that means we could have reused an idle socket if
we didn't commit this transaction to a new socket.

[willchan no longer on Chromium, 2009-04-29 23:05:15]

http://codereview.chromium.org/99205/diff/1/3
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/99205/diff/1/3#newcode128
Line 128: DCHECK(i != group_map_.end());
On 2009/04/29 22:48:50, wtc wrote:
> Perhaps we should return 0 if i == group_map_.end().
> 
> The condition asserted by this DCHECK seems unnecessary
> (i.e., the correctness of the code may not require this
> condition), but I haven't looked at this code for a while...

The correctness of the code does require this.  We're using an active socket in
a connection group.  If the group to which this socket belongs does not actually
exist in the map, then the code is wrong.  Most importantly, the
active_socket_count variable is not being tracked correctly.  I'm making the
assumption that there is only the one caller in HttpNetworkTransaction.  If
there are more callers in the future, we may have to revisit this assumption.

http://codereview.chromium.org/99205/diff/1/4
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/99205/diff/1/4#newcode76
Line 76: int NumIdleSocketsInGroup(const std::string group_name) const;
On 2009/04/29 22:48:50, wtc wrote:
> Please add a comment.
> 
> The group_name parameter should be a const reference.
> 
> To be consistent with the idle_socket_count method above,
> we should rename this method IdleSocketCountInGroup.

Done.

[jar (doing other things), 2009-04-29 23:26:06]

The histogram addition LGTM.
I did not review the rest of code.