Socket pools: Remove redundant seach for a group in GetLoadState.

Socket pools:  Remove redundant seach for a group in GetLoadState.

This was inspired by jank in the RDH method that calls the function,
but will almost certainly have no effect on the jank.

Also switch to using a range loop in GetLoadState as well.

BUG=455952
Committed: https://crrev.com/642054a983320b9d4ce80d6fa93e5221883a6b04
Cr-Commit-Position: refs/heads/master@{#318584}

[mmenke, 2015-02-28 00:16:27]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2015-02-28 00:16:28]

This is not aimed at fixing the jank, it's just fixing up an issue I noticed
while thinking about this method.


Also, about worst case performance of this method:  Ignoring sync XHRs, this has
a maximum branching factor of 6.  In the worst case of HTTPS over and HTTPS
proxy with 6 sockets.  So I believe that goes through 3 pools (SSL, proxy, and
transport - at least I don't think it goes through two layers of SSL pools -
proxy layer does the proxy SSL itself, right?).

So that gives us 6^3 calls to GetLoadState per request.  Since we have 6
request, that's 6^4 = 1296 calls to GetLoadState at the transport connect job
layer.  The 6^3 searches for the group those sockets belong to may be about as
bad, depending on the number of groups.  If there are more requests, we notice
they don't have a corresponding ConnectJobs at the top layer pool, so the
problem doesn't blow up any further.

Even if that's not what's causing the jank we're seeing, that seems bad, but
fixing it seems like enough of a pain that we may want to just keep it.  :(

Possible fixes:
1) Pick the first ConnectJob in the set for all Handles.  Main problem is it's a
set, so it can go backwards.
2) Pick the oldest.  Problem:  We'd have to track the oldest, so we can't use a
set.
3) Pick one and stick with it.  This would be the oldest, until the oldest is
removed, and then it would be a random one.  Seems acceptable, just don't like
having the extra pointer in every group to do this.
4)  Pick the nth oldest for the nth highest priority handle in the queue.  This
can probably be done with the same code as 2)...Which means it also has the same
downside.

Conclusion:  Leave well enough alone for now.

[eroman, 2015-02-28 00:33:27]

LGTM.

In response to your comments:


  (1) Can't the situation be worse than 6^3 since although there are maximum of
6 per group, there can be many more pending connect jobs?

  (2) Can you file a bug for the performance? I agree we don't need to address
it now, but I think it would be good to have it documented and in our bug DB.

https://codereview.chromium.org/966943005/diff/20001/net/socket/client_socket...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/966943005/diff/20001/net/socket/client_socket...
net/socket/client_socket_pool_base.cc:566: DCHECK(group_it != group_map_.end());
I suggest changing this to a CHECK() instead, since previously this would have
passed in our code. And could remove the CHECK() after it got some coverage in
field.

[mmenke, 2015-02-28 00:45:35]

On 2015/02/28 00:33:27, eroman wrote:
> LGTM.
> 
> In response to your comments:
> 
> 
>   (1) Can't the situation be worse than 6^3 since although there are maximum
of
> 6 per group, there can be many more pending connect jobs?

ConnectJobs only establish connections.  We don't create more if we have more
requests.  Instead, we have more than 6 ClientSocketHandles (Which represent the
incoming requests), owned by the HttpStreamFactoryImpl::Jobs.  Think you may be
getting job types confused.

>   (2) Can you file a bug for the performance? I agree we don't need to address
> it now, but I think it would be good to have it documented and in our bug DB.

Happy to file a bug, just want to hear your response to my response to your
previous comment, to see if I'm missing anything about worst case blowup.

>
https://codereview.chromium.org/966943005/diff/20001/net/socket/client_socket...
> File net/socket/client_socket_pool_base.cc (right):
> 
>
https://codereview.chromium.org/966943005/diff/20001/net/socket/client_socket...
> net/socket/client_socket_pool_base.cc:566: DCHECK(group_it !=
group_map_.end());
> I suggest changing this to a CHECK() instead, since previously this would have
> passed in our code. And could remove the CHECK() after it got some coverage in
> field.

Done.

[eroman, 2015-02-28 00:57:57]

Not sure on the possible fixes. Based on my lack of understanding I would
clearly need to go read the code before making any suggestions :)

[mmenke, 2015-02-28 02:12:32]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-02-28 02:12:33]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/966943005/#ps60001 (title:
"Update comment")

[mmenke, 2015-02-28 02:13:40]

On 2015/02/28 00:57:57, eroman wrote:
> Not sure on the possible fixes. Based on my lack of understanding I would
> clearly need to go read the code before making any suggestions :)

I actually meant I wanted to know if I was missing something when I was saying a
6^3 blowup.  :)

[commit-bot: I haz the power, 2015-02-28 02:14:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/966943005/60001

[commit-bot: I haz the power, 2015-02-28 02:54:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-28 02:55:41]

Patchset 4 (id:??) landed as
https://crrev.com/642054a983320b9d4ce80d6fa93e5221883a6b04
Cr-Commit-Position: refs/heads/master@{#318584}