Switch the TCP reads on Windows to use non-blocking/non-async I/O.

Switch the TCP reads on Windows to use non-blocking/non-async I/O.
The Overlapped I/O was introducing delays when the networking stack
did not have enough data to fill the receive buffer.

This can be seen when loading pssplayground.com/ksimbili/webp.html
using a DSL connection profile on WebPagetest:
http://www.webpagetest.org/result/120830_MS_414849a6aa055bb853e7e5d51e1b29d8/
and manifests and increasingly long Time to First Byte for requests
further down the waterfall (expected values are < 90ms and it was 
going over 150ms).

It is configured as a 50% field trial and can be forced through the 
command-line for testing:
    --overlapped-reads=on - default/existing behavior
    --overlapped-reads=off - new read implementation

Trial-specific histograms are reported for page load times and
http request times.  Specifically:
    PLT.Abandoned
    PLT.LoadType
    PLT.BeginToFinish_NormalLoad
    PLT.BeginToFinish_LinkLoadNormal
    PLT.BeginToFinish_LinkLoadReload
    PLT.BeginToFinish_LinkLoadStaleOk
    Net.HttpJob.TotalTime
    Net.HttpJob.TotalTimeSuccess
    Net.HttpJob.TotalTimeCancel
    Net.HttpJob.TotalTimeCached
    Net.HttpJob.TotalTimeNotCached

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

[slamm_google, 2012-08-30 17:47:25]

Here are some teeny nits for you.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:31: const bool kUseNonBlockingSockets =
true;
Refer to the related but that explains the reason for this approach?

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:308: core_->socket_->HandleReadSignalled();
Signalled -> Signaled

The dictionary has it both ways, however, this file already uses the latter.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:742:
CreateNetLogSocketErrorCallback(net_error, os_error));
Indent one more space.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:765: int rv = WSARecv(socket_,
&core_->read_buffer_, 1, &num, &flags,
Add a comment to explain the 1 byte recv?

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:958: flags);
Fix indentation.

[pmeenan, 2012-08-30 17:54:30]

Thanks, Updated patch with the changes as well as using the Field Trials
framework will be coming along shortly.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:31: const bool kUseNonBlockingSockets =
true;
On 2012/08/30 17:47:25, slamm wrote:
> Refer to the related but that explains the reason for this approach?

Will be switching this to use an experiment once I figure out how to plumb that.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:308: core_->socket_->HandleReadSignalled();
On 2012/08/30 17:47:25, slamm wrote:
> Signalled -> Signaled
> 
> The dictionary has it both ways, however, this file already uses the latter.

Thanks.  Looked kind of strange.

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:765: int rv = WSARecv(socket_,
&core_->read_buffer_, 1, &num, &flags,
On 2012/08/30 17:47:25, slamm wrote:
> Add a comment to explain the 1 byte recv? 

Good catch.  That was left over from my other implementation which used a 1-byte
async read instead of the non-blocking wait.  Reverted it to do the full-length
receive.

[pmeenan, 2012-08-31 15:05:23]

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/1/net/socket/tcp_client_...
net/socket/tcp_client_socket_win.cc:765: int rv = WSARecv(socket_,
&core_->read_buffer_, 1, &num, &flags,
On 2012/08/30 17:54:31, pmeenan wrote:
> On 2012/08/30 17:47:25, slamm wrote:
> > Add a comment to explain the 1 byte recv? 
> 
> Good catch.  That was left over from my other implementation which used a
1-byte
> async read instead of the non-blocking wait.  Reverted it to do the
full-length
> receive.

Actually, this is 1 recv buffer, not the length and is part of the original code
:-)  The length is set within the structure itself.

[Ryan Sleevi, 2012-10-11 18:53:16]

Initial set of style nits. Will take a closer look at the implementation in a
second pass.

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/browser/chr...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:603: base::FieldTrial::Probability
kNBREAD_GROUP_PROBABILITY = 1;
Because these are not declared const, please do not use kConstantName naming (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...
)

As shown in other field trials, simply use variable naming rules (or declare
these as const, and use kDivisor / kOverlappedReadProbability )

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:608:
nbread_trial->AppendGroup("NBReadNonBlocking",
The names of these probability groups are non-intuitive (overly abbreviated).

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:614: }
nit: Please follow the C++ Guide for horizontal whitespace. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

"Wrapped parameters have a 4 space indent"

Note that in Chromium, this logic follows for all line continuations (much like
the C++ guide requires it for initializer lists and elsewhere), so that would
include assignment

int foo =
    bar();

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/renderer/pa...
File chrome/renderer/page_load_histograms.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/16001/chrome/renderer/pa...
chrome/renderer/page_load_histograms.cc:960:
"PLT.BeginToFinish_LinkLoadStaleOk", "NBReadImpact"),
Same comments elsewhere about over-abbreviation for "NBRead"

https://chromiumcodereview.appspot.com/10916016/diff/16001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/16001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:192: static bool disable_overlapped_reads_;
No need to make this a class static. Just have it be a bool in an unnamed
namespace in the current translation unit (eg: Line 31)

It should follow the Chromium global naming style (g_disable_overlapped_reads)

[pmeenan, 2012-10-11 20:16:18]

Thanks, uploaded a new patch with the styles cleaned up.

NBRead naming is gone for the field trials and everything references
OverlappedRead now (OverlappedReadImpact, OverlappedReadEnabled,
OverlappedReadDisabled).

[wtc, 2012-10-12 18:13:04]

pmeenan: thank you for the CL.

rvargas: could you also review the tcp_client_socket_win.{h,cc}
changes? Thanks.

[Ryan Sleevi, 2012-10-12 18:53:13]

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:33: #endif  // defined(OS_WIN)
style nit: Platform defines like go after the 'standard' headers (eg: move line
32 to between lines 38 and 39)

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:70:
trial->UseOneTimeRandomization();
random nit: would you mind deleting the two spaces here? (it should be two-space
indent, not four space)

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:595: void
ChromeBrowserFieldTrials::WindowsOverlappedTCPReadsFieldTrial() {
style: Please review this function for whitespace issues

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:602: }
style nit: the bracing style in this file is to omit { } for one-line
conditionals (see also line 613-615)

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:608: kDivisor,
"OverlappedReadEnabled", 2015, 1, 1, NULL));
That's a really long trial. Are you sure that's the desired time frame?

When do you expect to make a decision about whether this is correct? 3 months
from now? 6 months? Try to scope the trial to the point where you need to make a
decision.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:307: if (g_disable_overlapped_reads) {
BUG? What happens if overlapped reads are disabled after sockets have been
created? It seems like you'll get into a bad state.

My suggestion would be that each TCPClientSocketWin::Core, when created, stores
the value of whatever g_disable_overlapped_reads was, so that for the lifetime
of the socket, its state machine can never become confused.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:945: void
TCPClientSocketWin::DidSignalRead() {
style nit: The order in the implementation should match the order in the header.

Move this to line 1055 (after DidCompleteWrite)

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:948: int os_error = 0, flags = 0;
style nit: Chromium style is to have one variable declaration per line.

int os_error = 0;
int flags = 0;

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:951: &network_events);
style nit: whitespace

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:966: core_->WatchForRead();
Do you also need to be watching for FD_CLOSE? Will it be signalled as an FD_READ
of 0 in the event of an unclean close?

If it is signalled as FD_CLOSE, and we ignore it here, does that means we're
potentially holding on indefinitely to a socket that is effectively dead?

ISTM that this conditional should be much more aggressive about handling
expected values and explicitly failing for unhandled values.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1017: DoReadCallback(rv);
no core_->read_buffer_length_ = 0?

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1056: void
TCPClientSocketWin::DisableOverlappedReads() {
style nit: The order in the implementation should match the order in the header.

Move this to line 868 (after SetNoDelay)

[slamm, 2012-10-12 21:07:15]

Here are a few nits of my own.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/common/chro...
chrome/common/chrome_switches.cc:1550: // "off" switches to use non-blocking
reads and WSAEventSelect
English-nit: add periods to the ends of the sentences.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/renderer/pa...
File chrome/renderer/page_load_histograms.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/renderer/pa...
chrome/renderer/page_load_histograms.cc:935: if (disable_overlapped_reads) {
disable_overlapped_reads -> use_overlapped_read_histogram

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.h (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.h:75: // Experiment entry point for
non-blocking reads
Add a period.

[rvargas (doing something else), 2012-10-13 03:34:06]

I suggest adding field trial histograms somewhere under net to have another data
point.

Maybe close to Net.HttpJob.TotalTime

https://codereview.chromium.org/10916016/diff/19001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/10916016/diff/19001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:952: if (rv != SOCKET_ERROR) {
nit: invert the order if the condition.

https://codereview.chromium.org/10916016/diff/19001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:974: base::StatsCounter
read_bytes("tcp.read_bytes");
This block logically follows the recv call so it would be clear if it's done
before leaving that if()... which points me to the fact that we should extract
the common code that calls recv from here and TCPClientSocketWin::Read().

https://codereview.chromium.org/10916016/diff/19001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:983: rv = MapSystemError(os_error);
nit: It's a little funky that we can end up here with rv being a system error
and we overwrite it again.

[pmeenan, 2012-10-17 15:12:31]

Thanks for the great feedback.  I think I addressed all of the issues except for
the additional histograms in Net.  I'll take a look at those later today but in
the meantime the core code changes have been updated.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:33: #endif  // defined(OS_WIN)
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: Platform defines like go after the 'standard' headers (eg: move
line
> 32 to between lines 38 and 39)

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:70:
trial->UseOneTimeRandomization();
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> random nit: would you mind deleting the two spaces here? (it should be
two-space
> indent, not four space)

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:595: void
ChromeBrowserFieldTrials::WindowsOverlappedTCPReadsFieldTrial() {
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style: Please review this function for whitespace issues

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:602: }
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: the bracing style in this file is to omit { } for one-line
> conditionals (see also line 613-615)

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/browser/chr...
chrome/browser/chrome_browser_field_trials.cc:608: kDivisor,
"OverlappedReadEnabled", 2015, 1, 1, NULL));
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> That's a really long trial. Are you sure that's the desired time frame?
> 
> When do you expect to make a decision about whether this is correct? 3 months
> from now? 6 months? Try to scope the trial to the point where you need to make
a
> decision.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/common/chro...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/common/chro...
chrome/common/chrome_switches.cc:1550: // "off" switches to use non-blocking
reads and WSAEventSelect
On 2012/10/12 21:07:15, slamm wrote:
> English-nit: add periods to the ends of the sentences.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/renderer/pa...
File chrome/renderer/page_load_histograms.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/chrome/renderer/pa...
chrome/renderer/page_load_histograms.cc:935: if (disable_overlapped_reads) {
On 2012/10/12 21:07:15, slamm wrote:
> disable_overlapped_reads -> use_overlapped_read_histogram
> 

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:307: if (g_disable_overlapped_reads) {
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> BUG? What happens if overlapped reads are disabled after sockets have been
> created? It seems like you'll get into a bad state.
> 
> My suggestion would be that each TCPClientSocketWin::Core, when created,
stores
> the value of whatever g_disable_overlapped_reads was, so that for the lifetime
> of the socket, its state machine can never become confused.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:945: void
TCPClientSocketWin::DidSignalRead() {
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: The order in the implementation should match the order in the
header.
> 
> Move this to line 1055 (after DidCompleteWrite)

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:948: int os_error = 0, flags = 0;
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: Chromium style is to have one variable declaration per line.
> 
> int os_error = 0;
> int flags = 0;

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:951: &network_events);
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: whitespace

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:952: if (rv != SOCKET_ERROR) {
On 2012/10/13 03:34:06, rvargas wrote:
> nit: invert the order if the condition.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:966: core_->WatchForRead();
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> Do you also need to be watching for FD_CLOSE? Will it be signalled as an
FD_READ
> of 0 in the event of an unclean close?
> 
> If it is signalled as FD_CLOSE, and we ignore it here, does that means we're
> potentially holding on indefinitely to a socket that is effectively dead?
> 
> ISTM that this conditional should be much more aggressive about handling
> expected values and explicitly failing for unhandled values.

Added the FD_CLOSE handling.  FD_READ does not get signaled in the case of a
close.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:974: base::StatsCounter
read_bytes("tcp.read_bytes");
On 2012/10/13 03:34:06, rvargas wrote:
> This block logically follows the recv call so it would be clear if it's done
> before leaving that if()... which points me to the fact that we should extract
> the common code that calls recv from here and TCPClientSocketWin::Read().

This got a lot cleaner when I cleaned it up and it now follows the recv.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:983: rv = MapSystemError(os_error);
On 2012/10/13 03:34:06, rvargas wrote:
> nit: It's a little funky that we can end up here with rv being a system error
> and we overwrite it again.

Should be a lot cleaner now.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1017: DoReadCallback(rv);
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> no core_->read_buffer_length_ = 0?

Fixed.  The buffer length isn't used in the overlapped I/O code path but it's
cleaner to make sure it is reset and follows the state of the buffer pointer.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1056: void
TCPClientSocketWin::DisableOverlappedReads() {
On 2012/10/12 18:53:13, Ryan Sleevi wrote:
> style nit: The order in the implementation should match the order in the
header.
> 
> Move this to line 868 (after SetNoDelay)

Done.

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.h (right):

https://chromiumcodereview.appspot.com/10916016/diff/19001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.h:75: // Experiment entry point for
non-blocking reads
On 2012/10/12 21:07:15, slamm wrote:
> Add a period.

Done.

[wtc, 2012-10-18 23:17:10]

Review comments on patch set 7:

pmeenan: Thank you for the patch. I only reviewed
tcp_client_socket_win.{h,cc}. I think we need to handle
FD_CLOSE better. I have not used WSAEventSelect for non-blocking
reads before, so some of my comments below may be wrong.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:206: }

Could you move the ThrottleReadSize() method up to follow
Detach()? The Style Guide recommends that the methods be
declared as a group, followed by the data members. Thanks.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:208: // Remember the state of overlapped
reads for the duration of the socket

Nit: overlapped reads => g_disable_overlapped_reads

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:311: } else
if(core_->socket_->waiting_read_) {

Why is it necessary to test core_->socket_->waiting_read_?

Nit: use the getter method for waiting_read_ because we
use the waiting_connect() getter on line 309.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:372: }

I think we should wait until the first Read() call to do this.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:506: FD_CONNECT | FD_READ | FD_CLOSE);

I think we should wait until the first Read() call to watch
for FD_READ | FD_CLOSE.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:736: int flags = 0;

Nit: just pass 0 to recv().

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:775:
use_history_.set_was_used_to_convey_data();

Please make lines lines 772-775 look like lines 748-752.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:795: core_->WatchForRead();

Is it important to call core_->WatchForRead() after
everything has been set up?

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1014: core_->write_iobuffer_ = NULL;

Should we also set core_->write_buffer_length_  to 0 here,
as you do on line 978?

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1021: int flags = 0;

Just pass 0 to recv().

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1024: &network_events);

Align this with the first argument on the previous line.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1034: rv = MapSystemError(os_error);

Should we check if os_error is WSAEWOULDBLOCK here?
The MSDN page for WSAEventSelect says we should:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576(v=vs.85).aspx

  ... Nevertheless, a robust application must be prepared
  for the possibility that the event object is set and it
  issues a Windows Sockets call that returns WSAEWOULDBLOCK
  immediately. ...

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1057: // signaled but the network events
flags are all clear (0).

I guess this is because we receive FD_CLOSE when waiting_read_
is false. TCPClientSocketWin::Core::ReadDelegate::OnObjectSignaled
needs to handle that case. I think it can simply call
WSAEnumNetworkEvents (to reset the event object) and then
either record the error code, or just do nothing. The
next Read() or Write() call will return 0 or fail anyway.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.h (right):

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.h:75: // Experiment entry point for
non-blocking reads.

Nit: please be more direct, something like:
  // Perform reads in non-blocking mode instead of overlapped
  // mode. Used for experiments.

[pmeenan, 2012-10-19 20:49:40]

Thanks for the feedback.  Changes applied (plus a few comments).  Thanks
everyone for putting up with all my style guide issues - first patch for
anything Google related in 2 years :-)

I still have the additional histogram(s) to look at adding but if anyone sees
any remaining issues with the core patch please let me know.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.cc (right):

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:206: }
On 2012/10/18 23:17:10, wtc wrote:
> 
> Could you move the ThrottleReadSize() method up to follow
> Detach()? The Style Guide recommends that the methods be
> declared as a group, followed by the data members. Thanks.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:208: // Remember the state of overlapped
reads for the duration of the socket
On 2012/10/18 23:17:10, wtc wrote:
> 
> Nit: overlapped reads => g_disable_overlapped_reads

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:311: } else
if(core_->socket_->waiting_read_) {
On 2012/10/18 23:17:10, wtc wrote:
> 
> Why is it necessary to test core_->socket_->waiting_read_?
> 
> Nit: use the getter method for waiting_read_ because we
> use the waiting_connect() getter on line 309.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:372: }
On 2012/10/18 23:17:10, wtc wrote:
> 
> I think we should wait until the first Read() call to do this.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:506: FD_CONNECT | FD_READ | FD_CLOSE);
On 2012/10/18 23:17:10, wtc wrote:
> 
> I think we should wait until the first Read() call to watch
> for FD_READ | FD_CLOSE.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:736: int flags = 0;
On 2012/10/18 23:17:10, wtc wrote:
> 
> Nit: just pass 0 to recv().

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:775:
use_history_.set_was_used_to_convey_data();
On 2012/10/18 23:17:10, wtc wrote:
> 
> Please make lines lines 772-775 look like lines 748-752.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:795: core_->WatchForRead();
On 2012/10/18 23:17:10, wtc wrote:
> 
> Is it important to call core_->WatchForRead() after
> everything has been set up?

Sorry, wasn't 100% sure about the threading model.  If it is guaranteed that all
actions in here will always be on the same thread then it doesn't matter.  The
protection was just to avoid a potential race condition where the read event
would be signaled and processed before the buffers were initialized.

I can switch it back but logically it seems like you wouldn't want to start the
watching until you finished setting everything up.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1021: int flags = 0;
On 2012/10/18 23:17:10, wtc wrote:
> 
> Just pass 0 to recv().

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1024: &network_events);
On 2012/10/18 23:17:10, wtc wrote:
> 
> Align this with the first argument on the previous line.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1034: rv = MapSystemError(os_error);
On 2012/10/18 23:17:10, wtc wrote:
> 
> Should we check if os_error is WSAEWOULDBLOCK here?
> The MSDN page for WSAEventSelect says we should:
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms741576%28v=vs.85%29...
> 
>   ... Nevertheless, a robust application must be prepared
>   for the possibility that the event object is set and it
>   issues a Windows Sockets call that returns WSAEWOULDBLOCK
>   immediately. ...

Might as well for robustness.  The kind of flow they described with recv's being
done at multiple points in the code doesn't happen with Chrome so there should
never be a way to signal FD_READ when there isn't data available to read but if
I've learned anything working on windows is that it never behaves like you think
it should.

Done.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.cc:1057: // signaled but the network events
flags are all clear (0).
On 2012/10/18 23:17:10, wtc wrote:
> 
> I guess this is because we receive FD_CLOSE when waiting_read_
> is false. TCPClientSocketWin::Core::ReadDelegate::OnObjectSignaled
> needs to handle that case. I think it can simply call
> WSAEnumNetworkEvents (to reset the event object) and then
> either record the error code, or just do nothing. The
> next Read() or Write() call will return 0 or fail anyway.

I don't think so because the code at the time had the check for waiting_read
before even allowing the calls to come in here.

We only watch the read event when we're actually waiting for a read so any close
events that happen outside of a read request will not get detected until the
next socket operation anyway.

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
File net/socket/tcp_client_socket_win.h (right):

https://chromiumcodereview.appspot.com/10916016/diff/27001/net/socket/tcp_cli...
net/socket/tcp_client_socket_win.h:75: // Experiment entry point for
non-blocking reads.
On 2012/10/18 23:17:10, wtc wrote:
> 
> Nit: please be more direct, something like:
>   // Perform reads in non-blocking mode instead of overlapped
>   // mode. Used for experiments.

Done.

[pmeenan, 2012-10-22 18:55:50]

ok, just added the histograms around Net.HttpJob.TotalTime which is the last
piece I was looking to add so the patch should be complete at this point.  

I looked at the remaining histograms that Chrome sends and didn't see any others
that looked particularly interesting.  There are some SPDY histograms that may
be interesting but I think the HttpJob histograms will largely encompass those
as well.

[rvargas (doing something else), 2012-10-23 00:19:40]

Some extra nits.

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:68:
trial->UseOneTimeRandomization();
nit: indent 2 spaces.

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:603:
scoped_refptr<base::FieldTrial> nbread_trial(
nbread?

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.h:75: // TCP sockets on Windows
instead of overlapped I/O
nit: end with period.

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1545: // The default is "on" which matches the
existing behavior.
I'm a little confused by this. The "normal" pattern for field trials is to add a
command line to disable the trial, but the effect of this command line is to
force the client into the new behavior (basically get into the trial). What is
the use case?

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:268: read_buffer_length_(0),
match the declaration order.

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1027: rv = recv(socket_,
core_->read_iobuffer_->data(),
The bulk of this condition is basically the same code of Read() (with a little
massaging of the arguments and when to set the waiting_read).

It would be good to either call Read() from here, or extract that common code
into a method that is called from both places.

That said, it is fine to wait until we have results from the field trial (and we
are presumably removing the overlapped code) to do that change.

[pmeenan, 2012-10-23 14:29:54]

I went ahead and refactored the Read() code to be shared so that DidSignalRead()
can call it directly.  

The main thing preventing me from calling Read() directly was the DCHECK
protection against it being called while a read is pending and presumably we
want to keep that protection in place to make sure outside routines don't make
that mistake.  

That basically turned Read() into a wrapper around the new shared function just
with the DCHECKs in place.

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:68:
trial->UseOneTimeRandomization();
On 2012/10/23 00:19:40, rvargas wrote:
> nit: indent 2 spaces.

Done.

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:603:
scoped_refptr<base::FieldTrial> nbread_trial(
On 2012/10/23 00:19:40, rvargas wrote:
> nbread?

thx, missed that one.  Now overlapped_reads_trial.

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.h:75: // TCP sockets on Windows
instead of overlapped I/O
On 2012/10/23 00:19:40, rvargas wrote:
> nit: end with period.

Done.

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1545: // The default is "on" which matches the
existing behavior.
On 2012/10/23 00:19:40, rvargas wrote:
> I'm a little confused by this. The "normal" pattern for field trials is to add
a
> command line to disable the trial, but the effect of this command line is to
> force the client into the new behavior (basically get into the trial). What is
> the use case?

The command line can force the trial on or off (force it explicitly to run in
one of the modes).  This is needed to allow for explicit testing in the lab of
overlapped reads vs event select reads.  If the field trial was 100% then I
could get by with a disable flag but since it is 50% this is the only way to
deterministically select a specific behavior.

I can switch it to a disable flag and do testing on dev builds but it would be a
lot easier if I could also do lab testing on production builds with flash and
everything else built in.

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:268: read_buffer_length_(0),
On 2012/10/23 00:19:40, rvargas wrote:
> match the declaration order.

Done.

https://codereview.chromium.org/10916016/diff/45001/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1027: rv = recv(socket_,
core_->read_iobuffer_->data(),
On 2012/10/23 00:19:40, rvargas wrote:
> The bulk of this condition is basically the same code of Read() (with a little
> massaging of the arguments and when to set the waiting_read).
> 
> It would be good to either call Read() from here, or extract that common code
> into a method that is called from both places.
> 
> That said, it is fine to wait until we have results from the field trial (and
we
> are presumably removing the overlapped code) to do that change.

Done.

[rvargas (doing something else), 2012-10-24 18:48:09]

Thanks. LGTM.

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/10916016/diff/45001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:1545: // The default is "on" which matches the
existing behavior.
On 2012/10/23 14:29:54, pmeenan wrote:
> On 2012/10/23 00:19:40, rvargas wrote:
> > I'm a little confused by this. The "normal" pattern for field trials is to
add
> a
> > command line to disable the trial, but the effect of this command line is to
> > force the client into the new behavior (basically get into the trial). What
is
> > the use case?
> 
> The command line can force the trial on or off (force it explicitly to run in
> one of the modes).  This is needed to allow for explicit testing in the lab of
> overlapped reads vs event select reads.  If the field trial was 100% then I
> could get by with a disable flag but since it is 50% this is the only way to
> deterministically select a specific behavior.
> 
> I can switch it to a disable flag and do testing on dev builds but it would be
a
> lot easier if I could also do lab testing on production builds with flash and
> everything else built in.

Thanks for the explanation... I figured it was for testing, but I missed that it
can disable the experiment as well. That works fine.

[slamm, 2012-10-25 19:51:48]

More small stuff.

https://codereview.chromium.org/10916016/diff/46009/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/10916016/diff/46009/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:608: kOverlappedReadProbability);
Indent kOverlappedReadProbability under the first double-quote.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:314: if (core_->disable_overlapped_reads_) {
Change 313 to "else if".

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:729: return rv;
Drop rv?

return DoRead(buf, buf_len, callback);

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:897:
CreateNetLogSocketErrorCallback(net_error, os_error));
Indent one more space.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1033: if (network_events.lNetworkEvents &
FD_READ) {
Change 1032 to "else if".

[pmeenan, 2012-10-26 14:03:49]

https://codereview.chromium.org/10916016/diff/46009/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/10916016/diff/46009/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.cc:608: kOverlappedReadProbability);
On 2012/10/25 19:51:48, slamm wrote:
> Indent kOverlappedReadProbability under the first double-quote.

Done.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
File net/socket/tcp_client_socket_win.cc (right):

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:314: if (core_->disable_overlapped_reads_) {
On 2012/10/25 19:51:48, slamm wrote:
> Change 313 to "else if".

Done.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:729: return rv;
On 2012/10/25 19:51:48, slamm wrote:
> Drop rv?
> 
> return DoRead(buf, buf_len, callback);

Done.  Wasn't sure if convention allowed for returning a function call directly
but it looks like that is done elsewhere in the code.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:897:
CreateNetLogSocketErrorCallback(net_error, os_error));
On 2012/10/25 19:51:48, slamm wrote:
> Indent one more space.

Had to switch to a 4-space intent instead, one more space would have pushed it
past 80 columns.

Done.

https://codereview.chromium.org/10916016/diff/46009/net/socket/tcp_client_soc...
net/socket/tcp_client_socket_win.cc:1033: if (network_events.lNetworkEvents &
FD_READ) {
On 2012/10/25 19:51:48, slamm wrote:
> Change 1032 to "else if".

Done.

[pmeenan, 2012-10-26 14:05:47]

+ben@ for owners review on the experiment code, histograms and switches

[Ben Goodger (Google), 2012-10-26 19:37:55]

did rvargas review the chrome stuff too?

[pmeenan, 2012-10-26 19:51:45]

On 2012/10/26 19:37:55, Ben Goodger (Google) wrote:
> did rvargas review the chrome stuff too?

Yep, rvargas, Ryan Sleevi and slamm looked at the full patch set.  wtc only
looked at the net parts.

[Ben Goodger (Google), 2012-10-26 19:55:08]

owner lgtm then.

[commit-bot: I haz the power, 2012-10-31 13:07:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmeenan@google.com/10916016/54001

[commit-bot: I haz the power, 2012-10-31 15:39:26]

Change committed as 165170