Increase the sizes of the circular buffers used by SSLClientSocketNSS.

Increase the sizes of the circular buffers used by SSLClientSocketNSS
and SSLServerSocketNSS.

Larger buffers result in fewer Read() and Write() calls, improving
performance.

R=rsleevi@chromium.org,agl@chromium.org
BUG=69813
TEST=none

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

[wtc, 2012-09-07 23:18:54]

https://chromiumcodereview.appspot.com/10919167/diff/1/net/http/http_stream_p...
File net/http/http_stream_parser.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/http/http_stream_p...
net/http/http_stream_parser.cc:25: const size_t kRequestBodyBufferSize = 1 <<
14;  // 16KB <= XXX This is the largest write buffer.

This comment is intended to help code review. It won't be
checked in.

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:121: // Common Read output buffer sizes are
4KB and 8KB. Use a receive input buffer

I saw the 4KB and 8KB Read() input buffer sizes while debugging,
but could not track them down.

[Ryan Sleevi, 2012-09-08 01:14:56]

Implementation LGTM, but I'm concerned about the sizes chosen (mentioned below)

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:121: // Common Read output buffer sizes are
4KB and 8KB. Use a receive input buffer
On 2012/09/07 23:18:54, wtc wrote:
> 
> I saw the 4KB and 8KB Read() input buffer sizes while debugging,
> but could not track them down.

Considering this represents a network buffer, it seems more useful to ensure
that this is enough to handle a complete SSL packet.

Even if the client is only reading 4K at a time, if a server sent a full packet,
we'll need to read the full packet in before we can answer that 4K request.

NSS internally has a buffer sufficiently sized for the packet, but it means that
at 8KB, we're looking at two network reads to satisfy a 4KB client read.

Come to think of it, this would be a great place to add metrics. Filed
http://crbug.com/147401

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_server_...
File net/socket/ssl_server_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_server_...
net/socket/ssl_server_socket_nss.cc:44: static const int kSendBufferSize = 4096;
I suppose we'll want to tune these at some point.

[wtc, 2012-09-08 02:03:09]

rsleevi: you are right. I missed that point.

I reverted the kRecvBufferSize change and added a TODO comment.

[Ryan Sleevi, 2012-09-08 02:07:50]

https://chromiumcodereview.appspot.com/10919167/diff/10002/net/socket/ssl_cli...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/10002/net/socket/ssl_cli...
net/socket/ssl_client_socket_nss.cc:123: static const int kRecvBufferSize =
4096;
Sorry it wasn't clear - I would suggest the better answer would be ==
kSendBufferSize.

I'd rather make the memory tradeoff (at the cost of 32 - 128 client sockets),
particularly for ChromeOS, since every network read involves two thread hops.

Having it sized to packet size will, ideally, keep the thread hop to 1x in the
worst case.

[wtc, 2012-09-10 22:51:37]

rsleevi: thank you for the comments. Please review patch set 4.

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:125: // largest enough to hold 16KB + SSL
overhead (often 25 bytes).

Your comment caused me to realize that NSS sends one record at
a time in general:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

That's a more important reason for kSendBufferSize to be
17KB, independent of the amount of data that the Chrome
network stack writes to NSS at a time. So I removed this
comment.

[Ryan Sleevi, 2012-09-10 22:52:34]

LGTM!

[wtc, 2012-09-10 22:56:25]

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:125: // largest enough to hold 16KB + SSL
overhead (often 25 bytes).

To expand on my previous comment: if the network stack writes
more than 16KB to NSS, it doesn't seem necessary to make
kSendBufferSize larger than 17KB. On the other hand, if the
network stack writes less than 16KB at a time to NSS, then we
can reduce kSendBufferSize.

But I would not be surprised if I still missed something :-)

[commit-bot: I haz the power, 2012-09-10 23:06:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10919167/12002

[Ryan Sleevi, 2012-09-10 23:20:31]

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10919167/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:125: // largest enough to hold 16KB + SSL
overhead (often 25 bytes).
On 2012/09/10 22:56:25, wtc wrote:
> 
> To expand on my previous comment: if the network stack writes
> more than 16KB to NSS, it doesn't seem necessary to make
> kSendBufferSize larger than 17KB. On the other hand, if the
> network stack writes less than 16KB at a time to NSS, then we
> can reduce kSendBufferSize.
> 
> But I would not be surprised if I still missed something :-)

Right, I followed what you meant.

NSS's strategy is much like OpenSSL, in that it follows a growth strategy
(specifically, sslBuffer_Grow), so that buffers only grow up to the size
necessary to send or receive the current message, up to the maximum size.

This implementation is pessimistic, in that it immediately allocates the
(expected) maximum size. However, absent hard data about the right sizes, I
think this is correct for performance, and we can tune it down/later to adjust
for memory usage.

[commit-bot: I haz the power, 2012-09-11 00:59:43]

Change committed as 155889