Refactor pooled socket request code to prepare for implementation of pretend-to-preconnect.

Refactor pooled socket request code to prepare for implementation of pretend-to-preconnect.
For pretend-to-preconnect details, please see BUG=235361
I am trying to resolve 2 major issues:

1. InitSocketPoolHelper is responsible for too many things:

InitSocketPoolHelper is a all-in-one function which does all of the following:
- make connection_group identifier string
- decide which pool to use and make corresponding SocketParams
- call RequestSocketForPool/RequestSocketsForPool depending on the context

This patch separates the previous InitSocketPoolHelper function into the below components:
- HostPortPairFromUrlForSession
- ConnectionGroupForUrl
- InvokeWithSocketParamsAndPool
- RequestSocketForPool
- RequestSocketsForPool

Also, I tried to limit the scope of auto variables for clarity.

2. SSLSocketParams constructor accepting three SocketParams at once.

SSLSocketParams constructor accepts all kind of SocketParams at once, but
only one of them is used per instance.

BUG=235361
TEST=net_unittests

[kouhei (in TOK), 2013-04-25 07:14:00]

Could you review this patch?
Please forward if not appropriate.

[willchan no longer on Chromium, 2013-04-25 15:35:25]

+mmenke
On Apr 25, 2013 12:14 AM, <kouhei@chromium.org> wrote:

> Reviewers: willchan,
>
> Message:
> Could you review this patch?
> Please forward if not appropriate.
>
> Description:
> Refactor pooled socket request code to prepare for implementation of
> pretend-to-preconnect.
> For pretend-to-preconnect details, please see BUG=235361
>
> I am trying to resolve 2 major issues:
>
> 1. InitSocketPoolHelper is responsible for too many things:
>
> InitSocketPoolHelper is a all-in-one function which does all of the
> following:
> - make connection_group identifier string
> - decide which pool to use and make corresponding SocketParams
> - call RequestSocketForPool/**RequestSocketsForPool depending on the
> context
>
> This patch separates the previous InitSocketPoolHelper function into the
> below
> components:
> - HostPortPairFromUrlForSession
> - ConnectionGroupForUrl
> - InvokeWithSocketParamsAndPool
> - RequestSocketForPool
> - RequestSocketsForPool
>
> Also, I tried to limit the scope of auto variables for clarity.
>
> 2. SSLSocketParams constructor accepting three SocketParams at once.
>
> SSLSocketParams constructor accepts all kind of SocketParams at once, but
> only one of them is used per instance.
>
> BUG=235361
>
> Please review this at
https://codereview.chromium.**org/14471023/<https://codereview.chromium.org/1...
>
> SVN Base:
https://chromium.googlesource.**com/chromium/src.git@master<https://chromium....
>
> Affected files:
>   M net/socket/client_socket_pool.**h
>   M net/socket/client_socket_pool_**manager.cc
>   M net/socket/ssl_client_socket_**pool.h
>   M net/socket/ssl_client_socket_**pool.cc
>
>
>

[mmenke, 2013-04-25 15:50:42]

I'll try and get back to you later today.  I'm not as familiar with
ClientSocketPoolManager as I'd like, so there's a not insignificant chance it
won't be until tomorrow.

[mmenke, 2013-04-29 15:43:13]

Sorry for being slow.

I think we may be going a bit overboard on templates here with the
RequestSocketForPool functions.

Why is this a prerequisite for issue 235361?

[mmenke, 2013-04-29 16:15:46]

One other high[ish] level comment.

https://codereview.chromium.org/14471023/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/14471023/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_pool.h:52: SSLSocketParams(const
scoped_refptr<TransportSocketParams>& transport_params,
I think this is actually worse than the old code.  Using overloaded functions
obscures the fact that different constructors are being called.  The Google
style guide also discourages overloading.

If we go this route, my suggestion is to make these static functions with
different names that call into it.

e.g.:

SSLSocketParams::CreateForDirectConnection()

SSLSocketParams::CreateForHttpProxy()

SSLSocketParams::CreateForSocksProxy()



We should also make the old constructor private.

[kouhei (in TOK), 2013-04-30 01:38:33]

Updated patch. Could you review?

> Why is this a prerequisite for issue 235361?

I wanted to add pretend-to-preconnect logic, which mostly shares the same path.
However, adding another functionality to InitSocketPoolHelper seemed to
complicate the code too much. There are already 4 "if"-branches for preconnect,
and I didn't want to add another 4 for pretend-to-preconnect.

> I think we may be going a bit overboard on templates here with the
RequestSocketForPool functions.
For the RequestSocketForPool template functors, I couldn't figure out a way to
factor out the logic without using templates. SocketParams classes do not have
any base class in common.

Maybe we can cast SocketParams* to void*.
Do you have any idea?

https://codereview.chromium.org/14471023/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/14471023/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_pool.h:52: SSLSocketParams(const
scoped_refptr<TransportSocketParams>& transport_params,
On 2013/04/29 16:15:46, mmenke wrote:
> I think this is actually worse than the old code.  Using overloaded functions
> obscures the fact that different constructors are being called.  The Google
> style guide also discourages overloading.
> 
> If we go this route, my suggestion is to make these static functions with
> different names that call into it.
> 
> e.g.:
> 
> SSLSocketParams::CreateForDirectConnection()
> 
> SSLSocketParams::CreateForHttpProxy()
> 
> SSLSocketParams::CreateForSocksProxy()
> 
> 
> 
> We should also make the old constructor private.

Made static factory methods.

[mmenke, 2013-04-30 14:58:05]

On 2013/04/30 01:38:33, Kouhei_UENO wrote:
> Updated patch. Could you review?
> 
> > Why is this a prerequisite for issue 235361?
> 
> I wanted to add pretend-to-preconnect logic, which mostly shares the same
path.
> However, adding another functionality to InitSocketPoolHelper seemed to
> complicate the code too much. There are already 4 "if"-branches for
preconnect,
> and I didn't want to add another 4 for pretend-to-preconnect.
> 
> > I think we may be going a bit overboard on templates here with the
> RequestSocketForPool functions.
> For the RequestSocketForPool template functors, I couldn't figure out a way to
> factor out the logic without using templates. SocketParams classes do not have
> any base class in common.
> 
> Maybe we can cast SocketParams* to void*.
> Do you have any idea?

I was hoping you would.  I'll think about it.  Unfortunately, I'm on some
deadline pressure today, so probably won't be able to get back to you until
tomorrow afternoon - I'm sorry about being so slow on this.

[kouhei (in TOK), 2013-05-01 08:06:23]

> I'm sorry about being so slow on this.
No problem.

FYI: This is the "pretend-to-preconnect" patch I want to implement.
https://codereview.chromium.org/14749006/

I want to avoid the 4 identical "if"-branches seen in the above patch with
refactoring.

[mmenke, 2013-05-01 19:31:12]

https://codereview.chromium.org/14471023/diff/7001/net/socket/client_socket_p...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/14471023/diff/7001/net/socket/client_socket_p...
net/socket/client_socket_pool_manager.cc:308: class RequestSocketForPool {
I think that these are sufficiently ugly that we should just get rid of them. 
Creating a single templated function that takes both sets of parameters, and
figures out which to use, works.  It isn't pretty, but the nested templates like
this just seems to ugly to me.

In the other CL, I suggested another way of doing this that avoids the need for
the callback, and is less prone to regress.

I'm also not convinced that we gain a whole lot by measuring at the network
level instead of the predictor level.  I think that histograming there would get
us much more important / useful data (If the potentially preconnected site was
visited).  I don't think the number of preconnects that we would have used for
each motivation is at all useful without that knowledge, and I don't think it
adds a whole lot of value on top of that knowledge, either.

I also think it wouldn't be unreasonable to improve our histograms a bit and
just enable preconnecting in the new cases on Canary, individually, and keep an
eye on the histograms.  The network histograms I suggest (May or may not already
be present):  How often a request can be served by an existing socket (Either
one already connected, or one currently being connected), how long requests have
to wait for idle sockets, and how many sockets we connect but never use.  Higher
level histograms, like time per request, may also be worth looking at, but tend
to be very noisy.

If you disagree, I'm fine with a temporary load flag, though I don't want to
keep it around too long.