Add experimental option for TCP FastOpen....

Add experimental option for TCP FastOpen.

Use chrome.exe --enable-tcp-fastopen

BUG=none
TEST=none yet


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

[Mike Belshe, 2010-10-21 22:50:15]

Wan-Teh - added you to check the final portion of the sslclientsocket change; I
know you're otherwise overbooked on CL reviews.

[willchan no longer on Chromium, 2010-10-22 17:56:44]

LGTM as an experimental change, but wait for wtc to comment on the NSS code.

As an aside, I thought about it a bit more, and this will have some funky
interactions with late binding / preconnect, since we might be delaying the SYN
in those cases.

Also, it seems that this API is somewhat suboptimal, because we don't know if
the kernel supports FastOpen for the specific endpoint.  If we delay the SYN for
a host that doesn't support it, then we're paying a bad penalty.

This change has a lot of implications, so we should be careful about deploying
it further, but the experimental patch is fine.

http://codereview.chromium.org/4039003/diff/33001/34027
File net/socket/tcp_client_socket.h (right):

http://codereview.chromium.org/4039003/diff/33001/34027#newcode26
net/socket/tcp_client_socket.h:26: // Enable/disable experimental TCP FastOpen
option.
Since we're modularizing net more, I'm going to be a bit stricter about
documenting thread-safety.  Please note that this should only be called in net
startup, while we're still single-threaded.

http://codereview.chromium.org/4039003/diff/33001/34028
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4039003/diff/33001/34028#newcode222
net/socket/tcp_client_socket_libevent.cc:222: DCHECK_EQ(false,
tcp_fastopen_connected_);
Nit: DCHECK_EQ is a bit weird, how about DCHECK(!foo).

[Mike Belshe, 2010-10-22 18:24:58]

http://codereview.chromium.org/4039003/diff/33001/34027
File net/socket/tcp_client_socket.h (right):

http://codereview.chromium.org/4039003/diff/33001/34027#newcode26
net/socket/tcp_client_socket.h:26: // Enable/disable experimental TCP FastOpen
option.
On 2010/10/22 17:56:44, willchan wrote:
> Since we're modularizing net more, I'm going to be a bit stricter about
> documenting thread-safety.  Please note that this should only be called in net
> startup, while we're still single-threaded.

Done.

http://codereview.chromium.org/4039003/diff/33001/34028
File net/socket/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4039003/diff/33001/34028#newcode222
net/socket/tcp_client_socket_libevent.cc:222: DCHECK_EQ(false,
tcp_fastopen_connected_);
On 2010/10/22 17:56:44, willchan wrote:
> Nit: DCHECK_EQ is a bit weird, how about DCHECK(!foo).

Done.  I think you've nitted me like 100 times on DCHECK(), now you're nitting
to not use it!!   Will, I expect more consistency from you!

[Mike Belshe, 2010-10-22 18:26:11]

On 2010/10/22 17:56:44, willchan wrote:
> LGTM as an experimental change, but wait for wtc to comment on the NSS code.
> 
> As an aside, I thought about it a bit more, and this will have some funky
> interactions with late binding / preconnect, since we might be delaying the
SYN
> in those cases.

Agree.  I've spoken to the kernel threads about this.  I think we want to know
if we think FastOpen is possible to a site, and then we disable preconnect for
that connection.

> 
> Also, it seems that this API is somewhat suboptimal, because we don't know if
> the kernel supports FastOpen for the specific endpoint.  If we delay the SYN
for
> a host that doesn't support it, then we're paying a bad penalty.

Agree.

> 
> This change has a lot of implications, so we should be careful about deploying
> it further, but the experimental patch is fine.

Agree.

> 
> http://codereview.chromium.org/4039003/diff/33001/34027
> File net/socket/tcp_client_socket.h (right):
> 
> http://codereview.chromium.org/4039003/diff/33001/34027#newcode26
> net/socket/tcp_client_socket.h:26: // Enable/disable experimental TCP FastOpen
> option.
> Since we're modularizing net more, I'm going to be a bit stricter about
> documenting thread-safety.  Please note that this should only be called in net
> startup, while we're still single-threaded.
> 
> http://codereview.chromium.org/4039003/diff/33001/34028
> File net/socket/tcp_client_socket_libevent.cc (right):
> 
> http://codereview.chromium.org/4039003/diff/33001/34028#newcode222
> net/socket/tcp_client_socket_libevent.cc:222: DCHECK_EQ(false,
> tcp_fastopen_connected_);
> Nit: DCHECK_EQ is a bit weird, how about DCHECK(!foo).

[willchan no longer on Chromium, 2010-10-22 18:27:12]

On Fri, Oct 22, 2010 at 11:24 AM, <mbelshe@chromium.org> wrote:

>
> http://codereview.chromium.org/4039003/diff/33001/34027
> File net/socket/tcp_client_socket.h (right):
>
> http://codereview.chromium.org/4039003/diff/33001/34027#newcode26
> net/socket/tcp_client_socket.h:26: // Enable/disable experimental TCP
> FastOpen option.
> On 2010/10/22 17:56:44, willchan wrote:
>
>> Since we're modularizing net more, I'm going to be a bit stricter
>>
> about
>
>> documenting thread-safety.  Please note that this should only be
>>
> called in net
>
>> startup, while we're still single-threaded.
>>
>
> Done.
>
>
> http://codereview.chromium.org/4039003/diff/33001/34028
> File net/socket/tcp_client_socket_libevent.cc (right):
>
> http://codereview.chromium.org/4039003/diff/33001/34028#newcode222
> net/socket/tcp_client_socket_libevent.cc:222: DCHECK_EQ(false,
> tcp_fastopen_connected_);
> On 2010/10/22 17:56:44, willchan wrote:
>
>> Nit: DCHECK_EQ is a bit weird, how about DCHECK(!foo).
>>
>
> Done.  I think you've nitted me like 100 times on DCHECK(), now you're
> nitting to not use it!!   Will, I expect more consistency from you!


Hah, sorry if it feels inconsistent :P  I added the missing chromium
DCHECK_EQ() macro, so I'm in general for it.  But not with boolean.  The
advantage of DCHECK_EQ is that it prints the incorrect value.  For a
boolean, there's no value add here.  It just reads a bit weird :P


>
>
> http://codereview.chromium.org/4039003/show
>

[wtc, 2010-10-26 01:08:28]

I reviewed all the files with "ssl" in their names. They look
good to me.  Just some nits below.

http://codereview.chromium.org/4039003/diff/33001/34010
File net/socket/client_socket.h (right):

http://codereview.chromium.org/4039003/diff/33001/34010#newcode72
net/socket/client_socket.h:72: // Returns true if the underlying transport
socket is using TCP FastOpen.
Nit: can you document what TCP FastOpen is?

http://codereview.chromium.org/4039003/diff/33001/34020
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4039003/diff/33001/34020#newcode749
net/socket/ssl_client_socket_nss.cc:749: }
We should also set
    peername_initialized_ = true;
here.  See also line 1600 below.

Perhaps InitializeSSLPeerName() should set
    peername_initialized_ = true;

http://codereview.chromium.org/4039003/diff/33001/34020#newcode973
net/socket/ssl_client_socket_nss.cc:973: int rv = SSL_SetSockPeerID(nss_fd_,
const_cast<char*>(peer_id.c_str()));
BUG: int => SECStatus
(This is an existing bug, which you copied to this function.)

http://codereview.chromium.org/4039003/diff/33001/34021
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/4039003/diff/33001/34021#newcode77
net/socket/ssl_client_socket_nss.h:77: // Initializes PeerName in SSL.  Returns
a net error code.
Nit: PeerName => peer name
Also line 195 below, and
line 742 in ssl_client_socket_nss.cc.

[Mike Belshe, 2010-10-26 17:22:26]

thanks wtc!

http://codereview.chromium.org/4039003/diff/33001/34010
File net/socket/client_socket.h (right):

http://codereview.chromium.org/4039003/diff/33001/34010#newcode72
net/socket/client_socket.h:72: // Returns true if the underlying transport
socket is using TCP FastOpen.
On 2010/10/26 01:08:29, wtc wrote:
> Nit: can you document what TCP FastOpen is?

Done.

http://codereview.chromium.org/4039003/diff/33001/34020
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4039003/diff/33001/34020#newcode749
net/socket/ssl_client_socket_nss.cc:749: }
On 2010/10/26 01:08:29, wtc wrote:
> We should also set
>     peername_initialized_ = true;
> here.  See also line 1600 below.
> 
> Perhaps InitializeSSLPeerName() should set
>     peername_initialized_ = true;

Done.

http://codereview.chromium.org/4039003/diff/33001/34020#newcode973
net/socket/ssl_client_socket_nss.cc:973: int rv = SSL_SetSockPeerID(nss_fd_,
const_cast<char*>(peer_id.c_str()));
On 2010/10/26 01:08:29, wtc wrote:
> BUG: int => SECStatus
> (This is an existing bug, which you copied to this function.)

Done.

http://codereview.chromium.org/4039003/diff/33001/34021
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/4039003/diff/33001/34021#newcode77
net/socket/ssl_client_socket_nss.h:77: // Initializes PeerName in SSL.  Returns
a net error code.
On 2010/10/26 01:08:29, wtc wrote:
> Nit: PeerName => peer name
> Also line 195 below, and
> line 742 in ssl_client_socket_nss.cc.

Done.