Implement a read-size throttle within the TCP socket.

Implement a read-size throttle within the TCP socket.

Add a field-group trial for testing it.

BUG=none
TEST=none


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

[willchan no longer on Chromium, 2009-08-24 16:37:19]

On 2009/08/23 21:18:35, Mike Belshe wrote:
> 

This code is fine, although I think we should probably run it as an A/B
experiment.

[wtc, 2009-08-24 20:03:48]

Mike,

Thanks for your work on this issue!  I have two general
comments below.  I can help report the performance problem
to Microsoft.

http://codereview.chromium.org/173259/diff/1002/1003
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/173259/diff/1002/1003#newcode174
Line 174: // We do this because otherwise the async IO subsystem artifically
delays
We should report this issue to Microsoft.  I'm sure their
TCP/IP developers would like to hear about such performance
problems of their code.

We should open a bug report where we can describe details of
your experiments.  A potential problem with such patches is
that they may stay in our source tree forever because
- nobody knows how to determine when such patches are no
  longer necessary, or
- nobody wants to spend the time to prove such patches are
  no longer necessary.
So we should also make sure this patch will do no harm when
the TCP/IP stack has been fixed.

[Mike Belshe, 2009-08-24 22:06:52]

OK - fieldtrial code added.

Please take another look.

On 2009/08/24 16:37:19, willchan wrote:
> On 2009/08/23 21:18:35, Mike Belshe wrote:
> > 
> 
> This code is fine, although I think we should probably run it as an A/B
> experiment.

[Mike Belshe, 2009-08-24 22:07:42]

Thanks, wtc.  I put in a field-trial at Will's request, so that should tell us
how it compares with and without the fix.

I'll try to isolate a case for Microsoft.

On 2009/08/24 20:03:48, wtc wrote:
> Mike,
> 
> Thanks for your work on this issue!  I have two general
> comments below.  I can help report the performance problem
> to Microsoft.
> 
> http://codereview.chromium.org/173259/diff/1002/1003
> File net/socket/tcp_client_socket_win.cc (right):
> 
> http://codereview.chromium.org/173259/diff/1002/1003#newcode174
> Line 174: // We do this because otherwise the async IO subsystem artifically
> delays
> We should report this issue to Microsoft.  I'm sure their
> TCP/IP developers would like to hear about such performance
> problems of their code.
> 
> We should open a bug report where we can describe details of
> your experiments.  A potential problem with such patches is
> that they may stay in our source tree forever because
> - nobody knows how to determine when such patches are no
>   longer necessary, or
> - nobody wants to spend the time to prove such patches are
>   no longer necessary.
> So we should also make sure this patch will do no harm when
> the TCP/IP stack has been fixed.

[willchan no longer on Chromium, 2009-08-24 22:24:33]

Other than the annoying global object issue, LGTM.

http://codereview.chromium.org/173259/diff/1002/1003
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/173259/diff/1002/1003#newcode208
Line 208: use_slow_start_throttle_ = trial_->group() == trial_group_;
Sorry to be annoying, but trial_ is not a POD, so it shouldn't be made global
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo...).
 I think you probably need to do the same thing we do for most FieldTrials and
initialize them as local variables in browser_main.cc, and call a function that
sets a global variable that you read here (use_slow_start_throttle_ should
probably be global, since it only needs to be set once anyway).

[willchan no longer on Chromium, 2009-08-25 00:49:38]

lgtm, thanks!

[wtc, 2009-08-25 23:58:16]

LGTM.

http://codereview.chromium.org/173259/diff/1010/1011
File net/socket/tcp_client_socket_win.cc (right):

http://codereview.chromium.org/173259/diff/1010/1011#newcode124
Line 124: int ThrottleReadSize(int size) {
This method and the related data members should be in
TCPClientSocketWin.  The Core class should only contain
members that need to be preserved when we cancel a
TCPClientSocket that has an async IO operation in progress.