Modify the SPDY stream to be buffered....

Modify the SPDY stream to be buffered.

Discovered that passing small chunks of data to the renderer can cause the
renderer to get backlogged.  With SPDY, data chunks are always framed into
smallish chunks (say ~4KB).  So where HTTP would only send a couple of large
chunks to the renderer to satisfy a resource load, SPDY may send many, and
this was surprisingly slow.

BUG=none
TEST=SpdyNetworkTransactionTest.FullBuffer




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

[willchan no longer on Chromium, 2010-02-24 04:42:44]

http://codereview.chromium.org/652209/diff/1/3
File net/spdy/spdy_stream.cc (left):

http://codereview.chromium.org/652209/diff/1/3#oldcode220
net/spdy/spdy_stream.cc:220: OnClose(net::OK);
If we don't call OnClose() here, where does |response_status_| get set?  Are we
just relying on the default?  I'd prefer to be explicit.  I think I'd rather you
use OnClose() here.  Are you not using OnClose() because OnClose() doesn't
handle flushing the buffered reads?  I think you need to make it call
DoBufferedReadCallback() anyway, because what if we get a RST?  SpdySession will
call OnClose(), and the user will lose the buffered read.

http://codereview.chromium.org/652209/diff/1/3
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/1/3#newcode111
net/spdy/spdy_stream.cc:111: OnClose(net::OK);
Why is this necessary?

http://codereview.chromium.org/652209/diff/1/3#newcode267
net/spdy/spdy_stream.cc:267: DoCallback(response_status_);
Why did this make a difference here?  response_status_ should be equal to
status, right?

http://codereview.chromium.org/652209/diff/1/3#newcode354
net/spdy/spdy_stream.cc:354: LOG(INFO) << "DoBufferedreadCallback: " <<
this->stream_id();
s/DoBufferedread/DoBufferedRead/
I also find it slightly weird you use this->stream_id() instead of stream_id(),
but whatever.

http://codereview.chromium.org/652209/diff/1/3#newcode363
net/spdy/spdy_stream.cc:363: std::list<scoped_refptr<IOBufferWithSize>
>::iterator it;
This is probably cheap, so my comment is probably premature optimization, but if
we do end up buffering a lot, then recalculating the size each time is wasteful.
 Just wanted to note that here.

http://codereview.chromium.org/652209/diff/1/3#newcode375
net/spdy/spdy_stream.cc:375: if (rv == ERR_IO_PENDING)
Is this condition possible?

http://codereview.chromium.org/652209/diff/1/3#newcode381
net/spdy/spdy_stream.cc:381: if (user_callback_)
If we have a user_buffer_, then we should have a user_callback_, right?  Can't
you just merge this into the other condition?

[Mike Belshe, 2010-02-24 06:40:02]

As per discussion, the reason for the strangeness around OnClose and the
callbacks is because of the way server-push is layered in right now.  It's not
really working right.

I'm going to take a stab at fixing that, and then resume this change.

http://codereview.chromium.org/652209/diff/1/3
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/1/3#newcode267
net/spdy/spdy_stream.cc:267: DoCallback(response_status_);
It doesn't matter - changed back.

On 2010/02/24 04:42:44, willchan wrote:
> Why did this make a difference here?  response_status_ should be equal to
> status, right?

http://codereview.chromium.org/652209/diff/1/3#newcode354
net/spdy/spdy_stream.cc:354: LOG(INFO) << "DoBufferedreadCallback: " <<
this->stream_id();
removed this line.

On 2010/02/24 04:42:44, willchan wrote:
> s/DoBufferedread/DoBufferedRead/
> I also find it slightly weird you use this->stream_id() instead of
stream_id(),
> but whatever.

http://codereview.chromium.org/652209/diff/1/3#newcode363
net/spdy/spdy_stream.cc:363: std::list<scoped_refptr<IOBufferWithSize>
>::iterator it;
On 2010/02/24 04:42:44, willchan wrote:
> This is probably cheap, so my comment is probably premature optimization, but
if
> we do end up buffering a lot, then recalculating the size each time is
wasteful.

Of course, you are right.  I just didn't think this mattered...

>  Just wanted to note that here.

[Mike Belshe, 2010-02-24 21:05:12]

I looked again - the bug from server push that I thought I knew of was just
wrong.  I made a few small changes here to match your comments, and hopefully
this looks good to you too.

http://codereview.chromium.org/652209/diff/1/3
File net/spdy/spdy_stream.cc (left):

http://codereview.chromium.org/652209/diff/1/3#oldcode220
net/spdy/spdy_stream.cc:220: OnClose(net::OK);
I've added this back too.
On 2010/02/24 04:42:44, willchan wrote:
> If we don't call OnClose() here, where does |response_status_| get set?  Are
we
> just relying on the default?  I'd prefer to be explicit.  I think I'd rather
you
> use OnClose() here.  Are you not using OnClose() because OnClose() doesn't
> handle flushing the buffered reads?  I think you need to make it call
> DoBufferedReadCallback() anyway, because what if we get a RST?  SpdySession
will
> call OnClose(), and the user will lose the buffered read.

http://codereview.chromium.org/652209/diff/1/3
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/1/3#newcode111
net/spdy/spdy_stream.cc:111: OnClose(net::OK);
I've removed this.

On 2010/02/24 04:42:44, willchan wrote:
> Why is this necessary?

http://codereview.chromium.org/652209/diff/1/3#newcode363
net/spdy/spdy_stream.cc:363: std::list<scoped_refptr<IOBufferWithSize>
>::iterator it;
On 2010/02/24 06:40:02, Mike Belshe wrote:
> On 2010/02/24 04:42:44, willchan wrote:
> > This is probably cheap, so my comment is probably premature optimization,
but
> if
> > we do end up buffering a lot, then recalculating the size each time is
> wasteful.
> 
> Of course, you are right.  I just didn't think this mattered...  

I added a check so that it will stop looping once it has seen that the buffer
can be filled.

http://codereview.chromium.org/652209/diff/1/3#newcode375
net/spdy/spdy_stream.cc:375: if (rv == ERR_IO_PENDING)
On 2010/02/24 04:42:44, willchan wrote:
> Is this condition possible?

No. Added a DCHECK.

http://codereview.chromium.org/652209/diff/1/3#newcode381
net/spdy/spdy_stream.cc:381: if (user_callback_)
On 2010/02/24 04:42:44, willchan wrote:
> If we have a user_buffer_, then we should have a user_callback_, right?  Can't
> you just merge this into the other condition?

This still happens as part of the server-push case.

[wtc, 2010-02-24 22:48:48]

LGTM.  Please note the comment marked with "BUG" below,
which we discussed in person.

I asked some questions below.  Since I don't fully
understand the code, I'm relying on you to doublecheck
the relevant code.

http://codereview.chromium.org/652209/diff/2001/3002
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/2001/3002#newcode243
net/spdy/spdy_stream.cc:243: if (user_buffer_) {
Is it possible to have
  user_callback_ != NULL
  user_bufer_ == NULL
here?

In the original code, we will call DoCallback(OK) in that
case.

In the new code, we will do nothing in that case.

http://codereview.chromium.org/652209/diff/2001/3002#newcode244
net/spdy/spdy_stream.cc:244: // Handing small chunks of data to the renderer
creates measurable overhead
Nit: to be general (when our network stack is used by products
other than Chrome), it would be nice to avoid mentioning
"renderer" and use more a more general term such as "caller"
or "client".

http://codereview.chromium.org/652209/diff/2001/3002#newcode246
net/spdy/spdy_stream.cc:246: ScheduleBufferedReadCallback();
It seems that we can call DoBufferedReadCallback instead
of ScheduleBufferedReadCallback here, or something along
that line.

Right now we *always* do a PostDelayedTask here because of
the ScheduleBufferedReadCallback call.  It seems that we
can do better.

http://codereview.chromium.org/652209/diff/2001/3002#newcode369
net/spdy/spdy_stream.cc:369: return;
BUG: we should only return if we call ScheduleBufferedReadCallback.

http://codereview.chromium.org/652209/diff/2001/3002#newcode375
net/spdy/spdy_stream.cc:375: DCHECK(rv != ERR_IO_PENDING);
This could remain a CHECK, because if 'rv' is ERR_IO_PENDING,
things will break horribly later.

http://codereview.chromium.org/652209/diff/2001/3002#newcode380
net/spdy/spdy_stream.cc:380: if (user_callback_)
Can user_callback_ be NULL here?

http://codereview.chromium.org/652209/diff/2001/3001
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/652209/diff/2001/3001#newcode210
net/spdy/spdy_stream.h:210: bool buffered_read_callback_pending_;
Nit: add a blank line below this line.

[Mike Belshe, 2010-02-24 23:36:36]

http://codereview.chromium.org/652209/diff/2001/3002
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/2001/3002#newcode243
net/spdy/spdy_stream.cc:243: if (user_buffer_) {
Calling DoCallback when user_callback_ is NULL causes DoCallback to CHECK()
false.  So, no, it had better not have happened.

On 2010/02/24 22:48:48, wtc wrote:
> Is it possible to have
>   user_callback_ != NULL
>   user_bufer_ == NULL
> here?
> 
> In the original code, we will call DoCallback(OK) in that
> case.
> 
> In the new code, we will do nothing in that case.

http://codereview.chromium.org/652209/diff/2001/3002#newcode244
net/spdy/spdy_stream.cc:244: // Handing small chunks of data to the renderer
creates measurable overhead
On 2010/02/24 22:48:48, wtc wrote:
> Nit: to be general (when our network stack is used by products
> other than Chrome), it would be nice to avoid mentioning
> "renderer" and use more a more general term such as "caller"
> or "client".

Done.

http://codereview.chromium.org/652209/diff/2001/3002#newcode246
net/spdy/spdy_stream.cc:246: ScheduleBufferedReadCallback();
I added a ShouldWaitForMoreBufferedData() call to regulate this.  I'm not sure I
like it better- it is more complicated now :-(

On 2010/02/24 22:48:48, wtc wrote:
> It seems that we can call DoBufferedReadCallback instead
> of ScheduleBufferedReadCallback here, or something along
> that line.
> 
> Right now we *always* do a PostDelayedTask here because of
> the ScheduleBufferedReadCallback call.  It seems that we
> can do better.

http://codereview.chromium.org/652209/diff/2001/3002#newcode369
net/spdy/spdy_stream.cc:369: return;
Nice catch.

Fixed and added unit test which verifies it.

On 2010/02/24 22:48:48, wtc wrote:
> BUG: we should only return if we call ScheduleBufferedReadCallback.

http://codereview.chromium.org/652209/diff/2001/3002#newcode375
net/spdy/spdy_stream.cc:375: DCHECK(rv != ERR_IO_PENDING);
On 2010/02/24 22:48:48, wtc wrote:
> This could remain a CHECK, because if 'rv' is ERR_IO_PENDING,
> things will break horribly later.

Done.

http://codereview.chromium.org/652209/diff/2001/3002#newcode380
net/spdy/spdy_stream.cc:380: if (user_callback_)
Yes, in the server push case.

On 2010/02/24 22:48:48, wtc wrote:
> Can user_callback_ be NULL here?

[willchan no longer on Chromium, 2010-02-24 23:44:04]

I have to run to an appointment so I won't be able to review this
until later, but don't wait on me since wtc has ok'd it.  If I still
have any comments, I'll send them after the fact.

On Wed, Feb 24, 2010 at 3:36 PM,  <mbelshe@chromium.org> wrote:
>
> http://codereview.chromium.org/652209/diff/2001/3002
> File net/spdy/spdy_stream.cc (right):
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode243
> net/spdy/spdy_stream.cc:243: if (user_buffer_) {
> Calling DoCallback when user_callback_ is NULL causes DoCallback to
> CHECK() false.  So, no, it had better not have happened.
>
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> Is it possible to have
>>   user_callback_ != NULL
>>   user_bufer_ == NULL
>> here?
>
>> In the original code, we will call DoCallback(OK) in that
>> case.
>
>> In the new code, we will do nothing in that case.
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode244
> net/spdy/spdy_stream.cc:244: // Handing small chunks of data to the
> renderer creates measurable overhead
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> Nit: to be general (when our network stack is used by products
>> other than Chrome), it would be nice to avoid mentioning
>> "renderer" and use more a more general term such as "caller"
>> or "client".
>
> Done.
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode246
> net/spdy/spdy_stream.cc:246: ScheduleBufferedReadCallback();
> I added a ShouldWaitForMoreBufferedData() call to regulate this.  I'm
> not sure I like it better- it is more complicated now :-(
>
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> It seems that we can call DoBufferedReadCallback instead
>> of ScheduleBufferedReadCallback here, or something along
>> that line.
>
>> Right now we *always* do a PostDelayedTask here because of
>> the ScheduleBufferedReadCallback call.  It seems that we
>> can do better.
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode369
> net/spdy/spdy_stream.cc:369: return;
> Nice catch.
>
> Fixed and added unit test which verifies it.
>
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> BUG: we should only return if we call ScheduleBufferedReadCallback.
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode375
> net/spdy/spdy_stream.cc:375: DCHECK(rv != ERR_IO_PENDING);
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> This could remain a CHECK, because if 'rv' is ERR_IO_PENDING,
>> things will break horribly later.
>
> Done.
>
> http://codereview.chromium.org/652209/diff/2001/3002#newcode380
> net/spdy/spdy_stream.cc:380: if (user_callback_)
> Yes, in the server push case.
>
> On 2010/02/24 22:48:48, wtc wrote:
>>
>> Can user_callback_ be NULL here?
>
> http://codereview.chromium.org/652209
>

[wtc, 2010-02-25 01:06:07]

LGTM.  Feel free to to back to the original code.  See
my comment below.

http://codereview.chromium.org/652209/diff/3007/2011
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/3007/2011#newcode371
net/spdy/spdy_stream.cc:371: buffered_read_callback_pending_ = false;
I agree that calling DoBufferedReadCallback directly from
OnDataReceived makes the code more complicated, and could
actually be risky.

The risk is that DoBufferedReadCallback sets
buffered_read_callback_pending_ to false.  We may not want
to do that when we call DoBufferedReadCallback directly
(as opposed to through ScheduleBufferedReadCallback)
because there may actually be a pending buffered read
callback when we call DoBufferedReadCallback directly!

Mike, please ask me if my description of the risk above
is not clear.

[Mike Belshe, 2010-02-25 18:57:27]

http://codereview.chromium.org/652209/diff/3007/2011
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/3007/2011#newcode371
net/spdy/spdy_stream.cc:371: buffered_read_callback_pending_ = false;
I agree.  I reverted.  BTW - this case should be benign, because of the way the
callback works; but it is more fragile and I don't like it.  It doesn't seem
like a case worth optimizing.


On 2010/02/25 01:06:08, wtc wrote:
> I agree that calling DoBufferedReadCallback directly from
> OnDataReceived makes the code more complicated, and could
> actually be risky.
> 
> The risk is that DoBufferedReadCallback sets
> buffered_read_callback_pending_ to false.  We may not want
> to do that when we call DoBufferedReadCallback directly
> (as opposed to through ScheduleBufferedReadCallback)
> because there may actually be a pending buffered read
> callback when we call DoBufferedReadCallback directly!
> 
> Mike, please ask me if my description of the risk above
> is not clear.

[wtc, 2010-02-25 20:04:48]

LGTM.  Just two nits about comments.

http://codereview.chromium.org/652209/diff/2001/3002
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/2001/3002#newcode244
net/spdy/spdy_stream.cc:244: // Handing small chunks of data to the renderer
creates measurable overhead
I like this comment; it explains the motivation of not
calling ReadResponseBody directly.  I just wanted you to
avoid mentioning renderer.  Could you add it back and
just change "renderer" to "caller" or "consumer"?

http://codereview.chromium.org/652209/diff/2018/2020#newcode240
net/spdy/spdy_stream.cc:240: // Note that data may be received for a SpdyStream
prior to the user calling
Please update this comment.  It refers to the test for
user_callback_, which is no longer here.

[willchan no longer on Chromium, 2010-02-25 20:47:35]

LGTM

http://codereview.chromium.org/652209/diff/2018/2020
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/2018/2020#newcode353
net/spdy/spdy_stream.cc:353: bool SpdyStream::ShouldWaitForMoreBufferedData() {
I think this member function can be const

[Mike Belshe, 2010-02-25 23:30:16]

http://codereview.chromium.org/652209/diff/2018/2020
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/652209/diff/2018/2020#newcode240
net/spdy/spdy_stream.cc:240: // Note that data may be received for a SpdyStream
prior to the user calling
On 2010/02/25 20:04:49, wtc wrote:
> Please update this comment.  It refers to the test for
> user_callback_, which is no longer here.

Done.

http://codereview.chromium.org/652209/diff/2018/2020#newcode353
net/spdy/spdy_stream.cc:353: bool SpdyStream::ShouldWaitForMoreBufferedData() {
On 2010/02/25 20:47:35, willchan wrote:
> I think this member function can be const

Done.