Increase number of max sockets per group for WebSocket connections.

Increase number of max sockets per group for WebSocket connections.

This change makes ClientSocketPoolManager aware of SocketPoolType so it can
pick a different limit for normal socket pools and WebSocket socket pools.

As an initial (experimental) setting, number of max sockets per group for
WebSocket connections is raised to 30, and other limits are set to the same
value as normal socket pools. This configuration should be revisited later
when WebSocket protocol stack starts to work and a good amount of usage metrics
are collected.

BUG=118268
TEST=none


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

[Yuta Kitamura, 2012-03-20 19:24:34]

William and Matt, could you review this?

If net/ part is fine with you, I will find someone from chrome/ OWNERS for
chrome/ review.

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
File net/http/http_network_session.h (left):

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
net/http/http_network_session.h:18: #include
"net/socket/client_socket_pool_manager.h"
This line had to be removed to avoid cyclic #include dependency. Because of this
removal, similar #include lines had to be added in some other .cc files.

[willchan no longer on Chromium, 2012-03-20 22:33:28]

I only have a lame nit :) I'd prefer you do that, because it also conveniently
avoids the C++ issue of needing to define storage for a class static (including
consts).

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
net/http/http_network_session.h:89: static const int kNumSocketPoolTypes = 2;
Why don't you do NORMAL_SOCKET_POOL = 0, WEBSOCKET_SOCKET_POOL,
NUM_SOCKET_POOL_TYPES? That's a pretty standard technique.

[Yuta Kitamura, 2012-03-21 00:47:02]

William: Could you take another look?

John: Could you take a look at chrome/ change as an OWNER?

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
File net/http/http_network_session.h (right):

https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
net/http/http_network_session.h:89: static const int kNumSocketPoolTypes = 2;
On 2012/03/20 22:33:30, willchan wrote:
> Why don't you do NORMAL_SOCKET_POOL = 0, WEBSOCKET_SOCKET_POOL,
> NUM_SOCKET_POOL_TYPES? That's a pretty standard technique.

Done.

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/http/http_networ...
File net/http/http_network_session.cc (right):

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/http/http_networ...
net/http/http_network_session.cc:151: break;
This change was necessary to avoid a compiler warning ("switch cases are not
exhaustive").

[mmenke, 2012-03-21 14:24:29]

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
net/socket/client_socket_pool_manager.cc:29: };
Since we're specifying default values here, rather than requiring everything be
set before use, I suggest something along the lines of:

int g_max_sockets_per_pool[] = {
  256,  // NORMAL_SOCKET_POOL
  256   // WEBSOCKET_SOCKET_POOL
};

COMPILE_ASSERT(arraysize(g_max_sockets_per_pool),
                   HttpNetworkSession::NUM_SOCKET_POOL_TYPES),
               max_sockets_per_pool_length_mistmatch);

For here and the next two as well.

[mmenke, 2012-03-21 14:25:25]

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
net/socket/client_socket_pool_manager.cc:29: };
On 2012/03/21 14:24:34, Matt Menke wrote:
> Since we're specifying default values here, rather than requiring everything
be
> set before use, I suggest something along the lines of:
> 
> int g_max_sockets_per_pool[] = {
>   256,  // NORMAL_SOCKET_POOL
>   256   // WEBSOCKET_SOCKET_POOL
> };
> 
> COMPILE_ASSERT(arraysize(g_max_sockets_per_pool),
>                    HttpNetworkSession::NUM_SOCKET_POOL_TYPES),
>                max_sockets_per_pool_length_mistmatch);
> 
> For here and the next two as well.

Err, that should be:

int g_max_sockets_per_pool[] = {
  256,  // NORMAL_SOCKET_POOL
  256   // WEBSOCKET_SOCKET_POOL
};

COMPILE_ASSERT(arraysize(g_max_sockets_per_pool) ==
                   HttpNetworkSession::NUM_SOCKET_POOL_TYPES),
               max_sockets_per_pool_length_mistmatch);

[Yuta Kitamura, 2012-03-21 17:30:12]

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
File net/socket/client_socket_pool_manager.cc (right):

https://chromiumcodereview.appspot.com/9764003/diff/8001/net/socket/client_so...
net/socket/client_socket_pool_manager.cc:29: };
On 2012/03/21 14:25:25, Matt Menke wrote:
> On 2012/03/21 14:24:34, Matt Menke wrote:
> > Since we're specifying default values here, rather than requiring everything
> be
> > set before use, I suggest something along the lines of:
> > 
> > int g_max_sockets_per_pool[] = {
> >   256,  // NORMAL_SOCKET_POOL
> >   256   // WEBSOCKET_SOCKET_POOL
> > };
> > 
> > COMPILE_ASSERT(arraysize(g_max_sockets_per_pool),
> >                    HttpNetworkSession::NUM_SOCKET_POOL_TYPES),
> >                max_sockets_per_pool_length_mistmatch);
> > 
> > For here and the next two as well.
> 
> Err, that should be:
> 
> int g_max_sockets_per_pool[] = {
>   256,  // NORMAL_SOCKET_POOL
>   256   // WEBSOCKET_SOCKET_POOL
> };
> 
> COMPILE_ASSERT(arraysize(g_max_sockets_per_pool) ==
>                    HttpNetworkSession::NUM_SOCKET_POOL_TYPES),
>                max_sockets_per_pool_length_mistmatch);

Done.

[jam, 2012-03-21 17:31:48]

On 2012/03/21 00:47:02, Yuta Kitamura wrote:
> William: Could you take another look?
> 
> John: Could you take a look at chrome/ change as an OWNER?

lgtm. i'm usually overloaded by reviews, so for rubberstamp lgtms like this
please send to others in the owners file.
> 
>
https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
> File net/http/http_network_session.h (right):
> 
>
https://chromiumcodereview.appspot.com/9764003/diff/1/net/http/http_network_s...
> net/http/http_network_session.h:89: static const int kNumSocketPoolTypes = 2;
> On 2012/03/20 22:33:30, willchan wrote:
> > Why don't you do NORMAL_SOCKET_POOL = 0, WEBSOCKET_SOCKET_POOL,
> > NUM_SOCKET_POOL_TYPES? That's a pretty standard technique.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9764003/diff/8001/net/http/http_networ...
> File net/http/http_network_session.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9764003/diff/8001/net/http/http_networ...
> net/http/http_network_session.cc:151: break;
> This change was necessary to avoid a compiler warning ("switch cases are not
> exhaustive").

[Yuta Kitamura, 2012-03-21 17:43:08]

On 2012/03/21 17:31:48, John Abd-El-Malek wrote:
> lgtm. i'm usually overloaded by reviews, so for rubberstamp lgtms like this
> please send to others in the owners file.

Will do. Thanks.

[willchan no longer on Chromium, 2012-03-21 17:53:20]

lgtm

On 2012/03/21 17:43:08, Yuta Kitamura wrote:
> On 2012/03/21 17:31:48, John Abd-El-Malek wrote:
> > lgtm. i'm usually overloaded by reviews, so for rubberstamp lgtms like this
> > please send to others in the owners file.
> 
> Will do. Thanks.

[mmenke, 2012-03-21 17:53:43]

LGTM

[commit-bot: I haz the power, 2012-03-21 18:23:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yutak@chromium.org/9764003/15003

[commit-bot: I haz the power, 2012-03-21 20:29:54]

Change committed as 128044