When using NSS, only schedule transport socket reads when the transport buffer is empty.

When using NSS, only schedule transport socket reads when the transport buffer is empty.

Rather then attempting to constantly keep the NSS memio constantly saturated by constantly
issuing socket reads, only attempt to read data from the underlying transport when the
NSS memio has been fully drained. This lets the OS manage the socket buffers and can
significantly reduce the number of Read()s issued against the transport socket in
optimal conditions.

R=wtc@chromium.org
BUG=166741
TEST=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174136

[Ryan Sleevi, 2012-12-19 18:52:46]

wtc: This is the NSS side to what you reviewed for OpenSSL in
https://codereview.chromium.org/11647005/

This was particularly onerous behaviour for Linux/ChromeOS, because the
multi-threaded SSL meant we were constantly doing context switches with small
buffers. Using the larger buffers should reduce the cross-task posting.

[wtc, 2012-12-20 02:41:22]

Patch set 1 LGTM.

This is a thought-provoking CL. It's difficult to determine
if it will improve performance.

1. It avoids the problem of reading from transport_socket_
with a small buffer size when the circular buffer in
nss_bufs_ happens to have little room before wrapping
around.

A corollary of this CL is that the circular buffer readbuf
in nss_bufs_ can be simplified because its data will never
wrap around.

2. Unless the server sends to the client unanticipated
data, and sends the data a few bytes at a time, it seems
that the current behavior merely "time-shifts" the
transport socket reads, and therefore should not hurt
performance. In fact, it could reduce latency because
it is a form of "pre-reading".

So it seems profitable to read from the transport socket
whenever the readbuf in nss_bufs_ is empty, whether there
was an unsatisfied read request from the higher layer or
not.

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.c
File net/base/nss_memio.c (right):

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.c#newcode234
net/base/nss_memio.c:234: PR_SetError(PR_WOULD_BLOCK_ERROR, 0);

Perhaps only this code path (when a read request of a higher
layer fails with PR_WOULD_BLOCK_ERROR) should result in
secret->read_requested being set to 'len'. But what we set
secret->read_requested to when mb->last_err is nonzero
probably doesn't matter in practice.

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.c#newcode236
net/base/nss_memio.c:236: } else {

Don't add "else" here. Just do

         return -1;
     }

+    secret->read_requested = 0;
     return rv;
 }

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.h
File net/base/nss_memio.h (right):

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.h#newcode57
net/base/nss_memio.h:57: int memio_GetReadRequest(memio_Private *secret);

The specification of this function is not very clear unless
I look at its implementation in the .c file.

All we need is a boolean function that tells us whether a
read was requested by a higher layer when the buffer was
empty.

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2105: // unavailable.

This comment is also not very clear. It could be interpreted
to mean:
  if NSS requests 5 bytes but nss_bufs_ has only 2 bytes,
  then memio_GetReadRequest should return 3 because 3 bytes
  are unavailable.

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2114: // Known Issue: While only reading
|requested| bytes is the correct

Don't add this comment. This isn't an issue. The current code
is also correct because it buffers any data not consumed by
the higher layer.

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2122: // cause issues.

The SSLClientSocketNSS constructor is documented to take ownership of its
transport_socket argument, so the scenario of reusing the transport socket for
non-SSL traffic after SSL is gracefully shut down cannot happen. We can update
the comment in ssl_client_socket_nss.h to state that explicitly.

[Ryan Sleevi, 2012-12-20 02:55:58]

wtc: It will reduce the number of calls into send()/WSARecv, so on systems where
syscalls are "expensive", it allows us to spend less time in the kernel and to
supply it larger buffers. That's the main motivation for this change - reduce
context switches. On Linux/ChromeOS, this will also reduce the number of tasks
we're posting back to the IOThread, which will allow our 'work completed'
callback to run sooner (right now, the Transport::Read() call is started before
the SSL::OnReadCompleted(), due to ordering of tasks posted)

Once work starts on http://crbug.com/166903 , SSLClientSocket* will become more
aggressive in consuming SSL records from memio_Private/transport_bio_, which
will allow us to start experimenting with larger buffer sizes. The work for
166903 will reduce the number of thread switches overall that need to happen.

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.h
File net/base/nss_memio.h (right):

https://codereview.chromium.org/11633021/diff/1/net/base/nss_memio.h#newcode57
net/base/nss_memio.h:57: int memio_GetReadRequest(memio_Private *secret);
On 2012/12/20 02:41:22, wtc wrote:
> 
> The specification of this function is not very clear unless
> I look at its implementation in the .c file.
> 
> All we need is a boolean function that tells us whether a
> read was requested by a higher layer when the buffer was
> empty.

While yes, we only need a bool, I typed it as an int both to match the OpenSSL
implementation and to allow for the 'proper' behaviour from the point of view of
a generic socket API, which would be to have the transport only read 'as much'
data as requested.

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/11633021/diff/1/net/socket/ssl_client_socket_...
net/socket/ssl_client_socket_nss.cc:2105: // unavailable.
On 2012/12/20 02:41:22, wtc wrote:
> 
> This comment is also not very clear. It could be interpreted
> to mean:
>   if NSS requests 5 bytes but nss_bufs_ has only 2 bytes,
>   then memio_GetReadRequest should return 3 because 3 bytes
>   are unavailable.

That is correct, because that is what it does.

[Ryan Sleevi, 2012-12-20 04:42:32]

Comments updated, using CQ

[commit-bot: I haz the power, 2012-12-20 07:41:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/11633021/6001

[commit-bot: I haz the power, 2012-12-20 11:51:06]

Change committed as 174136