Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11644088:
SPDY - implement greedy approach to read all the data and process it (Closed)
|
Created:
8 years ago by ramant (doing other things) Modified:
7 years, 10 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, willchan no longer on Chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSPDY - implement greedy approach to read all the data and process it
from the ClientSocket until we block.
This is more of a first cut at the greedy approach to see if it
improves the performance on the mobile.
spdy_session_test_util.* files have the common code between
SpdySessionSpd2Test and SpdySessionSpd3Test. Will move other common
code in a separate CL. Created this file to reduce errors and
to avoid duplicating of code.
R=willchan
BUG=166958
TEST=network unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181390
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 16
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Patch Set 20 : #Patch Set 21 : #Patch Set 22 : #Patch Set 23 : #Patch Set 24 : #
Total comments: 12
Patch Set 25 : #Patch Set 26 : #Patch Set 27 : #Patch Set 28 : #Patch Set 29 : #Patch Set 30 : #Patch Set 31 : #Patch Set 32 : #
Total comments: 6
Patch Set 33 : #Patch Set 34 : #Patch Set 35 : #
Total comments: 24
Patch Set 36 : #Patch Set 37 : #Patch Set 38 : #Patch Set 39 : #Patch Set 40 : #
Total comments: 10
Patch Set 41 : #Messages
Total messages: 34 (0 generated)
Redirecting to Ryan. Btw, does this handle the case where we try to Read() more but encounter a connection close? Do we store the connection closed event so that the next time we read from the socket we notify the framer accordingly? https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... net/spdy/spdy_session.cc:890: read_buffer_.get(), don't we need to increment the offset we read into this buffer?
To answer will's comment: Yes, it does. That's because this does a loop for (Read -> Process -> Read) until Read returns Pending. Rather than doing a loop of (Read -> Read) -> Process. While an argument can be made about having SpdySession::Read be more greedy, so that it notifies SpdyStreams less, as discussed over IM and with rch@, we can probably attempt that clean-up separately, since we want to be dispatching events to SpdyStreams before the state of the SpdySession is altered. https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... net/spdy/spdy_session.cc:890: read_buffer_.get(), On 2012/12/24 00:19:35, willchan wrote: > don't we need to increment the offset we read into this buffer? No, because ProcessBytesRead drains |read_buffer_| https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... net/spdy/spdy_session.cc:909: } while (result == OK && can_do_more); Like I expressed Friday, I'm a slightly concerned with this pattern, if only because it's significantly different from how we normally handle reads and the state machine. In particular, the way this is currently implemented requires duplicating the error handling logic (lines 894-903 and lines 789-795), splitting the handling of values < 0 from line 905 (eg: not handled) and line 789 (invoked by the callback from line 782) It does seem like putting in an explicit state machine logic to flip between these states makes it clearer to the reader the flow, and lets you keep error handling consistently in the same place. It will also make the concept map between (TCPClientSocket, SSLClientSocket, SpdyStream, ProxyService, QuicHttpStream, etc) and this code.
https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... net/spdy/spdy_session.cc:909: } while (result == OK && can_do_more); On 2012/12/24 05:38:44, Ryan Sleevi wrote: > Like I expressed Friday, I'm a slightly concerned with this pattern, if only > because it's significantly different from how we normally handle reads and the > state machine. > > In particular, the way this is currently implemented requires duplicating the > error handling logic (lines 894-903 and lines 789-795), splitting the handling > of values < 0 from line 905 (eg: not handled) and line 789 (invoked by the > callback from line 782) > > It does seem like putting in an explicit state machine logic to flip between > these states makes it clearer to the reader the flow, and lets you keep error > handling consistently in the same place. It will also make the concept map > between (TCPClientSocket, SSLClientSocket, SpdyStream, ProxyService, > QuicHttpStream, etc) and this code. +1 on an explict STATE_DO_READ/STATE_DO_READ_COMPLETE to remove the duplication.
On Sun, Dec 23, 2012 at 9:38 PM, <rsleevi@chromium.org> wrote: > To answer will's comment: Yes, it does. > > That's because this does a loop for (Read -> Process -> Read) until Read > returns > Pending. Rather than doing a loop of (Read -> Read) -> Process. Just to be clear, this changelist doesn't reduce the number of user callbacks invoked then, right? AIUI, it simply reduces the queueing delay (since we don't yield to the MessageLoop in between invocations of user callbacks). If I'm understanding it correctly, then it probably won't affect CPU load on the IO thread, which I believe is the primary bottleneck we're trying to solve here. > > While an argument can be made about having SpdySession::Read be more greedy, > so > that it notifies SpdyStreams less, as discussed over IM and with rch@, we > can > probably attempt that clean-up separately, since we want to be dispatching > events to SpdyStreams before the state of the SpdySession is altered. > > > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > File net/spdy/spdy_session.cc (right): > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > net/spdy/spdy_session.cc:890: read_buffer_.get(), > On 2012/12/24 00:19:35, willchan wrote: >> >> don't we need to increment the offset we read into this buffer? > > > No, because ProcessBytesRead drains |read_buffer_| > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > net/spdy/spdy_session.cc:909: } while (result == OK && can_do_more); > Like I expressed Friday, I'm a slightly concerned with this pattern, if > only because it's significantly different from how we normally handle > reads and the state machine. > > In particular, the way this is currently implemented requires > duplicating the error handling logic (lines 894-903 and lines 789-795), > splitting the handling of values < 0 from line 905 (eg: not handled) and > line 789 (invoked by the callback from line 782) > > It does seem like putting in an explicit state machine logic to flip > between these states makes it clearer to the reader the flow, and lets > you keep error handling consistently in the same place. It will also > make the concept map between (TCPClientSocket, SSLClientSocket, > SpdyStream, ProxyService, QuicHttpStream, etc) and this code. > > https://chromiumcodereview.appspot.com/11644088/
The *number* of callbacks is a separate issue. With this CL, we should be issuing the same number of callbacks as if an 8K (or even 64K) buffer had been filled. Reducing the overall # of callbacks requires making SpdyStream transparently greedier. If SpdyStream defers its reads for any reason, then this CL will reduce callbacks because the Session will have more opportunity to fill the Stream's buffers before the next Stream::Read. However, if it doesn't, then yes, this CL serves to reduce the queuing delay of packets, likely eliminating 4*N ms of delay, where N is the number of loop iterations/SSL records that can be read. On Dec 24, 2012 8:45 AM, "William Chan (陈智昌)" <willchan@chromium.org> wrote: > On Sun, Dec 23, 2012 at 9:38 PM, <rsleevi@chromium.org> wrote: > > To answer will's comment: Yes, it does. > > > > That's because this does a loop for (Read -> Process -> Read) until Read > > returns > > Pending. Rather than doing a loop of (Read -> Read) -> Process. > > Just to be clear, this changelist doesn't reduce the number of user > callbacks invoked then, right? AIUI, it simply reduces the queueing > delay (since we don't yield to the MessageLoop in between invocations > of user callbacks). If I'm understanding it correctly, then it > probably won't affect CPU load on the IO thread, which I believe is > the primary bottleneck we're trying to solve here. > > > > > While an argument can be made about having SpdySession::Read be more > greedy, > > so > > that it notifies SpdyStreams less, as discussed over IM and with rch@, > we > > can > > probably attempt that clean-up separately, since we want to be > dispatching > > events to SpdyStreams before the state of the SpdySession is altered. > > > > > > > > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > > File net/spdy/spdy_session.cc (right): > > > > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > > net/spdy/spdy_session.cc:890: read_buffer_.get(), > > On 2012/12/24 00:19:35, willchan wrote: > >> > >> don't we need to increment the offset we read into this buffer? > > > > > > No, because ProcessBytesRead drains |read_buffer_| > > > > > https://chromiumcodereview.appspot.com/11644088/diff/16001/net/spdy/spdy_sess... > > net/spdy/spdy_session.cc:909: } while (result == OK && can_do_more); > > Like I expressed Friday, I'm a slightly concerned with this pattern, if > > only because it's significantly different from how we normally handle > > reads and the state machine. > > > > In particular, the way this is currently implemented requires > > duplicating the error handling logic (lines 894-903 and lines 789-795), > > splitting the handling of values < 0 from line 905 (eg: not handled) and > > line 789 (invoked by the callback from line 782) > > > > It does seem like putting in an explicit state machine logic to flip > > between these states makes it clearer to the reader the flow, and lets > > you keep error handling consistently in the same place. It will also > > make the concept map between (TCPClientSocket, SSLClientSocket, > > SpdyStream, ProxyService, QuicHttpStream, etc) and this code. > > > > https://chromiumcodereview.appspot.com/11644088/ >
And the answer is that: Yes, this WILL reduce the number of callbacks. While SpdyStream does *not* coalesce buffers/notifications ( see https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... ) WILL coalesce reads, see https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... Because ScheduleBufferedReadCallback posts a task (which also happens to be delayed, although simply posting a task is sufficient), and further attempts to coalesce, we should see a marked decrease in the number of callbacks with this code. That's because by having SpdySession not yield the loop while there is still network data to process, then none of the SpdyHttpStreams' callbacks will have run, so they should saturate the users read request. Note that the implementation in SpdyStream is effectively a sleep(1), so that may be better suited to becoming simply a PostTask to avoid the unnecessary sleep now that SpdySession is behaving greedily.
On 2012/12/24 18:22:27, Ryan Sleevi wrote: > And the answer is that: Yes, this WILL reduce the number of callbacks. > > While SpdyStream does *not* coalesce buffers/notifications ( see > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > ) WILL coalesce reads, see > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... > > Because ScheduleBufferedReadCallback posts a task (which also happens to be > delayed, although simply posting a task is sufficient), and further attempts to > coalesce, we should see a marked decrease in the number of callbacks with this > code. > > That's because by having SpdySession not yield the loop while there is still > network data to process, then none of the SpdyHttpStreams' callbacks will have > run, so they should saturate the users read request. > > Note that the implementation in SpdyStream is effectively a sleep(1), so that > may be better suited to becoming simply a PostTask to avoid the unnecessary > sleep now that SpdySession is behaving greedily. +1 to sleevi (my tests also showed the above). Would like to histogram how many bytes (in k bytes) we read in SpdySession. Will test multiple tabs open and all of them downloading large files and then another tab open Google+ and keep scrolling. My intention of the CL is to share code with piatek to see if it is fixes his problem. sleevi: will change the code to make state machine changes,
On 2012/12/24 19:25:55, ramant wrote: > On 2012/12/24 18:22:27, Ryan Sleevi wrote: > > And the answer is that: Yes, this WILL reduce the number of callbacks. > > > > While SpdyStream does *not* coalesce buffers/notifications ( see > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > > ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > > ) WILL coalesce reads, see > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... > > > > Because ScheduleBufferedReadCallback posts a task (which also happens to be > > delayed, although simply posting a task is sufficient), and further attempts > to > > coalesce, we should see a marked decrease in the number of callbacks with this > > code. > > > > That's because by having SpdySession not yield the loop while there is still > > network data to process, then none of the SpdyHttpStreams' callbacks will have > > run, so they should saturate the users read request. > > > > Note that the implementation in SpdyStream is effectively a sleep(1), so that > > may be better suited to becoming simply a PostTask to avoid the unnecessary > > sleep now that SpdySession is behaving greedily. > > +1 to sleevi (my tests also showed the above). > > Would like to histogram how many bytes (in k bytes) we read in SpdySession. Will > test multiple tabs open and all of them downloading large files and then another > tab open Google+ and keep scrolling. > > My intention of the CL is to share code with piatek to see if it is fixes his > problem. > > sleevi: will change the code to make state machine changes, The following comment worries me little bit (mbelshe's comment about infinite buffering). If I have a local server that serves data very fast and if I am downloading 4GB of data file, will we run into issues? https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream...
On Dec 24, 2012 11:25 AM, <rtenneti@chromium.org> wrote: > > Reviewers: willchan, Ryan Hamilton, Ryan Sleevi, > > Message: > > On 2012/12/24 18:22:27, Ryan Sleevi wrote: >> >> And the answer is that: Yes, this WILL reduce the number of callbacks. > > >> While SpdyStream does *not* coalesce buffers/notifications ( see > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... >> >> ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... >> >> ) WILL coalesce reads, see > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... > >> Because ScheduleBufferedReadCallback posts a task (which also happens to be >> delayed, although simply posting a task is sufficient), and further attempts > > to >> >> coalesce, we should see a marked decrease in the number of callbacks with this >> code. > > >> That's because by having SpdySession not yield the loop while there is still >> network data to process, then none of the SpdyHttpStreams' callbacks will have >> run, so they should saturate the users read request. > > >> Note that the implementation in SpdyStream is effectively a sleep(1), so that >> may be better suited to becoming simply a PostTask to avoid the unnecessary >> sleep now that SpdySession is behaving greedily. > > > +1 to sleevi (my tests also showed the above). > > Would like to histogram how many bytes (in k bytes) we read in SpdySession. Will > test multiple tabs open and all of them downloading large files and then another > tab open Google+ and keep scrolling. Why? I mean that non-facetiously. What would you expect the data to show? How would that change the approach here? What could be done differently. I think there is less concern here with how much SpdySession gets in a single read - for Google properties, its safe to say 1350 bytes. It is equally unimportant the size of the 8K buffer, since we process frames as soon as we read them. The real benefits are in how SpdySession notifies its SpdyStream - can we reduce the number of small per-frame buffers we are allocating in both the Session and Streams, and reduce the overall notifications from Streams. But that is entirely orthogonal and independent of how SpdySession behaves w/r/t Read here. > > My intention of the CL is to share code with piatek to see if it is fixes his > problem. > > sleevi: will change the code to make state machine changes, > > Description: > SPDY - implement greedy approach to read all the data and process it > from the ClientSocket until we block. > > This is more of a first cut at the greedy approach to see if it > improves the performance on the mobile. > > R=willchan > BUG=166958 > TEST=network unittests > > Please review this at https://chromiumcodereview.appspot.com/11644088/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src/ > > Affected files: > M net/spdy/spdy_session.h > M net/spdy/spdy_session.cc > > > Index: net/spdy/spdy_session.cc > =================================================================== > --- net/spdy/spdy_session.cc (revision 174489) > +++ net/spdy/spdy_session.cc (working copy) > @@ -779,13 +779,20 @@ > > read_pending_ = false; > > + bool can_do_more = ProcessBytesRead(bytes_read); > + > + if (can_do_more) > + ReadSocket(); > +} > + > +bool SpdySession::ProcessBytesRead(int bytes_read) { > if (bytes_read <= 0) { > // Session is tearing down. > net::Error error = static_cast<net::Error>(bytes_read); > if (bytes_read == 0) > error = ERR_CONNECTION_CLOSED; > CloseSessionOnError(error, true, "bytes_read is <= 0."); > - return; > + return false; > } > > bytes_received_ += bytes_read; > @@ -810,9 +817,7 @@ > if (buffered_spdy_framer_->state() == SpdyFramer::SPDY_DONE) > buffered_spdy_framer_->Reset(); > } > - > - if (state_ != CLOSED) > - ReadSocket(); > + return state_ != CLOSED; > } > > void SpdySession::OnWriteComplete(int result) { > @@ -868,41 +873,41 @@ > } > > net::Error SpdySession::ReadSocket() { > - if (read_pending_) > - return OK; > + bool can_do_more = false; > + net::Error result; > + do { > + if (read_pending_) > + return OK; > > - if (state_ == CLOSED) { > - NOTREACHED(); > - return ERR_UNEXPECTED; > - } > + if (state_ == CLOSED) { > + NOTREACHED(); > + return ERR_UNEXPECTED; > + } > > - CHECK(connection_.get()); > - CHECK(connection_->socket()); > - int bytes_read = connection_->socket()->Read( > - read_buffer_.get(), > - kReadBufferSize, > - base::Bind(&SpdySession::OnReadComplete, base::Unretained(this))); > - switch (bytes_read) { > - case 0: > - // Socket is closed! > - CloseSessionOnError(ERR_CONNECTION_CLOSED, true, "bytes_read is 0."); > - return ERR_CONNECTION_CLOSED; > - case net::ERR_IO_PENDING: > - // Waiting for data. Nothing to do now. > - read_pending_ = true; > - return ERR_IO_PENDING; > - default: > - // Data was read, process it. > - // Schedule the work through the message loop to avoid recursive > - // callbacks. > - read_pending_ = true; > - MessageLoop::current()->PostTask( > - FROM_HERE, > - base::Bind(&SpdySession::OnReadComplete, > - weak_factory_.GetWeakPtr(), bytes_read)); > - break; > - } > - return OK; > + CHECK(connection_.get()); > + CHECK(connection_->socket()); > + int bytes_read = connection_->socket()->Read( > + read_buffer_.get(), > + kReadBufferSize, > + base::Bind(&SpdySession::OnReadComplete, base::Unretained(this))); > + switch (bytes_read) { > + case 0: > + // Socket is closed! > + CloseSessionOnError(ERR_CONNECTION_CLOSED, true, "bytes_read is 0."); > + result = ERR_CONNECTION_CLOSED; > + break; > + case net::ERR_IO_PENDING: > + // Waiting for data. Nothing to do now. > + read_pending_ = true; > + result = ERR_IO_PENDING; > + break; > + default: > + can_do_more = ProcessBytesRead(bytes_read); > + result = OK; > + break; > + } > > + } while (result == OK && can_do_more); > + return result; > } > > void SpdySession::WriteSocketLater() { > Index: net/spdy/spdy_session.h > =================================================================== > --- net/spdy/spdy_session.h (revision 174489) > +++ net/spdy/spdy_session.h (working copy) > @@ -422,6 +422,7 @@ > > // IO Callbacks > void OnReadComplete(int result); > + bool ProcessBytesRead(int bytes_read); > void OnWriteComplete(int result); > > // Send relevant SETTINGS. This is generally called on connection setup. > >
On Dec 24, 2012 11:36 AM, <rtenneti@chromium.org> wrote: > > On 2012/12/24 19:25:55, ramant wrote: >> >> On 2012/12/24 18:22:27, Ryan Sleevi wrote: >> > And the answer is that: Yes, this WILL reduce the number of callbacks. >> > >> > While SpdyStream does *not* coalesce buffers/notifications ( see >> > > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... >> >> > ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at >> > > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... >> >> > ) WILL coalesce reads, see >> > > > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... >> >> > >> > Because ScheduleBufferedReadCallback posts a task (which also happens to be >> > delayed, although simply posting a task is sufficient), and further attempts >> to >> > coalesce, we should see a marked decrease in the number of callbacks with > > this >> >> > code. >> > >> > That's because by having SpdySession not yield the loop while there is still >> > network data to process, then none of the SpdyHttpStreams' callbacks will > > have >> >> > run, so they should saturate the users read request. >> > >> > Note that the implementation in SpdyStream is effectively a sleep(1), so > > that >> >> > may be better suited to becoming simply a PostTask to avoid the unnecessary >> > sleep now that SpdySession is behaving greedily. > > >> +1 to sleevi (my tests also showed the above). > > >> Would like to histogram how many bytes (in k bytes) we read in SpdySession. > > Will >> >> test multiple tabs open and all of them downloading large files and then > > another >> >> tab open Google+ and keep scrolling. > > >> My intention of the CL is to share code with piatek to see if it is fixes his >> problem. > > >> sleevi: will change the code to make state machine changes, > > > The following comment worries me little bit (mbelshe's comment about infinite > buffering). If I have a local server that serves data very fast and if I am > downloading 4GB of data file, will we run into issues? Flow control is supposed to handle this, by defining an upper bound for both servers and clients. Options to limit the max # of streams handle the overall case. Do you have reason to believe those won't apply? > > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... > > https://chromiumcodereview.appspot.com/11644088/
>Flow control is supposed to handle this ... > Do you have reason to believe those won't apply? :-). I agree (and hoping) flow control will take care of it. Testing to be 100% sure. Was bitten by flow control during infancy. :-).
You are 100% correct and no disagreements (I am slightly paranoid and wanted to prove to myself by testing) :-).
OH man, you terrified me when you said sleep(1)! http://linux.die.net/man/3/sleep says it's seconds. I checked and the code delays a 1ms, whew :) That's fascinating about the coalescing, I totally forgot about it. That's great! Well, kinda :P It means we again yield back to the MessageLoop =/ I agree that it looks like the coalescing / callback reduction is done correctly. PS: Stop working so hard, enjoy your Xmas Eve kiddos :) On Mon, Dec 24, 2012 at 10:22 AM, <rsleevi@chromium.org> wrote: > And the answer is that: Yes, this WILL reduce the number of callbacks. > > While SpdyStream does *not* coalesce buffers/notifications ( see > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > ), the call to SpdyHttpStream (from delegate_->OnDataReceived(...) at > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_stream.cc&t... > ) WILL coalesce reads, see > https://code.google.com/searchframe#OAMlx_jo-ck/src/net/spdy/spdy_http_stream... > > Because ScheduleBufferedReadCallback posts a task (which also happens to be > delayed, although simply posting a task is sufficient), and further attempts > to > coalesce, we should see a marked decrease in the number of callbacks with > this > code. > > That's because by having SpdySession not yield the loop while there is still > network data to process, then none of the SpdyHttpStreams' callbacks will > have > run, so they should saturate the users read request. > > Note that the implementation in SpdyStream is effectively a sleep(1), so > that > may be better suited to becoming simply a PostTask to avoid the unnecessary > sleep now that SpdySession is behaving greedily. > > https://codereview.chromium.org/11644088/
On 2012/12/24 19:40:28, Ryan Sleevi wrote: > ... > Flow control is supposed to handle this, by defining an upper bound for > both servers and clients. Options to limit the max # of streams handle the > overall case. > > Do you have reason to believe those won't apply? > I think that conceptually, SPDY flow control could help this, but I think most clients (us included?) decided to set the inbounds SPDY flow control window to (effectively) infinite. This was supposed to prevent SPDY from throttling typical downloads. In this case, I *think* we might just be adding an unhelpful buffer at an odd point in the stack, and I'm not clear that SPDY flow control would be an overall win to apply (in its SPDY 3 incarnation). If we had a really fast link, and a sufficiently slow computer, we'd probably just buffer a ton of stuff in this browser at this layer, passing *nothing* up the stack. This could hurt browser memory (crash eventually?), and would leave us stuck in a tight synchronous receive loop (blocking the IO thread?). I think we should instead periodically (when we buffered enough) pass data up the stack. This would put back-pressure (if needed) on the TCP layer, to induce flow control, and also allow us to reduce jank in concurrent streams (and concurrent IO thread processing), by passing up data periodically. I think we may get worse results if we try to use SPDY flow control (at least with the current incarnation) at the client side. It is helpful (for efficiency) to coalesce data to largish buffers (16K? 32K?), but this code looks like it may go overboard, and (potentially) never yield. Am I off base about this?
On 2012/12/26 21:07:20, jar wrote: > On 2012/12/24 19:40:28, Ryan Sleevi wrote: > > ... > > Flow control is supposed to handle this, by defining an upper bound for > > both servers and clients. Options to limit the max # of streams handle the > > overall case. > > > > Do you have reason to believe those won't apply? > > <snip> > It is helpful (for efficiency) to coalesce data to largish buffers (16K? 32K?), > but this code looks like it may go overboard, and (potentially) never yield. > > Am I off base about this? If we're not setting reasonable flow control, then yes, I agree, this would be problematic. In that case, I agree, we should define some value of coalescing (32K? 64K? 16K?) before notifying callers. This is what SpdyStream is already trying to do at the SpdyStream level, so if our SpdySession buffer size is less than (# of SpdyStreams) * (size of buffers), then we're potentially wasting space at the SpdyStream level.
https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:802: } while (io_state_ != STATE_NONE && result != ERR_IO_PENDING); Design Suggestion: You can probably make it clearer how kMaxReadBytes relates by moving it into here, rather than being part of DoRead. That way, even if you add additional states, it's clear how much is processed at a time. int bytes_read = 0; do { switch (io_state_) { ... case STATE_DO_READ_COMPLETE: if (result > 0) bytes_read += result; result = DoReadComplete(result); ... } } while (io_state_ != STATE_NONE && result != ERR_IO_PENDING && bytes_read < kMaxReadBytes); if (bytes_read >= kMaxReadBytes) { result = ERR_IO_PENDING; MessageLoop::current()->PostTask(...); } return result; This lets you avoid having to track both bytes_read_ and read_pending_ on the class itself. It also makes it clear that kMaxReadBytes is the terminator for the processing loop overall - in case more states are introduced, for example. https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:828: } Here, it would be as simple as CHECK(...) return connection_->socket()->Read(...) [We do this in the Http layers as an idiom, for example] https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:843: io_state_ = STATE_NONE; If you're relying on this pattern to set io_state_ (and you seem to be mixing two patterns - having the loop set it on 789/790 like the SSL/TCP sockets do, but then setting it in the handlers here, like the HTTP streams do), then you should do it at line 836 to ensure that all early-returns have the proper state. Then you can drop lines 873 & 874. https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:879: io_state_ = STATE_DO_READ_COMPLETE; Normally we don't set the state in the bound callbacks, and we instead let code line like 807 handle the asynchronous restarting. https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:881: bytes_read_ = 0; My primary reason for suggesting the loop shuffling above was when I was reading this code, I missed the _ and was trying to figure out why you were clearing |bytes_read|. Had line 878 said something like "rv" (which is our normal shorthand), I probably would not have been as confused.
Added TRACE_EVENT in the latest patch (will remove them before submitting it). rch and I had a small discussion and implemented what we had talked about. Added new states to State and in DoRead added yield if we have read/processed too many bytes by calling StartRead via PostTask. Used a member variable to keep track of bytes_read_ (sleevi: hope that is ok). thanks raman https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:828: } On 2012/12/27 05:29:48, Ryan Sleevi wrote: > Here, it would be as simple as > CHECK(...) > return connection_->socket()->Read(...) > > [We do this in the Http layers as an idiom, for example] Done. https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:843: io_state_ = STATE_NONE; On 2012/12/27 05:29:48, Ryan Sleevi wrote: > If you're relying on this pattern to set io_state_ (and you seem to be mixing > two patterns - having the loop set it on 789/790 like the SSL/TCP sockets do, > but then setting it in the handlers here, like the HTTP streams do), then you > should do it at line 836 to ensure that all early-returns have the proper state. > Then you can drop lines 873 & 874. Done. https://chromiumcodereview.appspot.com/11644088/diff/23002/net/spdy/spdy_sess... net/spdy/spdy_session.cc:879: io_state_ = STATE_DO_READ_COMPLETE; On 2012/12/27 05:29:48, Ryan Sleevi wrote: > Normally we don't set the state in the bound callbacks, and we instead let code > line like 807 handle the asynchronous restarting. Done.
The logic here is subtle. Please add a unit test which verifies that SpdySession wil coalesce N synchronous reads together before it yields, as well as the case of mixed sync/async reads. https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... File net/spdy/spdy_session.cc (right): https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:375: net::Error error = static_cast<net::Error>(DoLoop(OK)); nit: int rv = DoLoop(OK); https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:814: read_pending_ = true; nit: I would move these two lines to DoRead. https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:821: if (bytes_read_ > kMaxReadBytes) { bytes_read_ is currently being set to 0 in DoLoop(). How does that interact with this expression? https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:830: DCHECK(!read_pending_); nit Should this be moved above the if()? https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:850: net::Error error = static_cast<net::Error>(bytes_read); nit: please get rid of this cast and assignment. https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:854: return ERR_CONNECTION_CLOSED; Shouldn't this say "return error;"? https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:858: bytes_read_ += bytes_read; I'm confused about the difference between bytes_read_ and total_bytes_read_. Can you add a comment? https://chromiumcodereview.appspot.com/11644088/diff/50005/net/spdy/spdy_sess... net/spdy/spdy_session.cc:1081: if (state_ != STATE_CLOSED) { if (!IsConnected()) {
The logic here is subtle. Please add a unit test which verifies that SpdySession wil coalesce N synchronous reads together before it yields, as well as the case of mixed sync/async reads.
PTAL. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:375: net::Error error = static_cast<net::Error>(DoLoop(OK)); On 2012/12/29 18:41:52, Ryan Hamilton wrote: > nit: int rv = DoLoop(OK); Done. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:814: read_pending_ = true; On 2012/12/29 18:41:52, Ryan Hamilton wrote: > nit: I would move these two lines to DoRead. Done. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:821: if (bytes_read_ > kMaxReadBytes) { On 2012/12/29 18:41:52, Ryan Hamilton wrote: > bytes_read_ is currently being set to 0 in DoLoop(). How does that interact > with this expression? We increment bytes_read_ in DoReadComplete. It is initialized to 0, before we start reading and we read continuously 32k bytes without yielding and then we yield by calling DoLoop (from StartRead) and we start reading continuously for another 32k bytes. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:830: DCHECK(!read_pending_); On 2012/12/29 18:41:52, Ryan Hamilton wrote: > nit Should this be moved above the if()? Done. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:850: net::Error error = static_cast<net::Error>(bytes_read); On 2012/12/29 18:41:52, Ryan Hamilton wrote: > nit: please get rid of this cast and assignment. "CloseSessionOnError(error,..." use net::Error. On windows, passing int for net::Error results in a compilation error. Sorry for the cast. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:854: return ERR_CONNECTION_CLOSED; On 2012/12/29 18:41:52, Ryan Hamilton wrote: > Shouldn't this say "return error;"? CloseSessionOnError is logging the error. Thought it is better to return what is current state of connection. If state is ERR_CONNECTION_CLOSED, higher level code knows the socket is closed and it doesn't perform extra reads (for example during tear down of tests). Done. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:858: bytes_read_ += bytes_read; On 2012/12/29 18:41:52, Ryan Hamilton wrote: > I'm confused about the difference between bytes_read_ and total_bytes_read_. > Can you add a comment? Added a comment in the header file. Done. https://codereview.chromium.org/11644088/diff/50005/net/spdy/spdy_session.cc#... net/spdy/spdy_session.cc:1081: if (state_ != STATE_CLOSED) { On 2012/12/29 18:41:52, Ryan Hamilton wrote: > if (!IsConnected()) { Changed it to !IsClosed() (in case connect failed). Done.
Other than the test, this looks good. I'm not sure, however, that these tests verify that the session yields as expected. In particular, I would have expected that in the second test you would need to run the message look in order to read the second chunk of data. Am I misunderstanding? https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right): https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4511: // yielding. How does this test verify that the session yields? It definitely verifies that trans->Read() returns synchronously, but I'm not sure that's the same thing. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4523: net::TestDataStream test_stream; nit: no need for test::, I think. (And in the next test) https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4526: char* payload_data = payload->data(); nit: move this line above the previous, and use payload_data instead of payload->data() as the first argument for GetBytes(). (And in the next test). https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4576: rv = trans->Read(buf, kPayloadSize, read_callback.callback()); nit: I would just do ASSERT_LE(0, rv); https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4589: MessageLoop::current()->RunUntilIdle(); Can you do this as the last thing line in this test? https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4600: // then reads more data after yielding. I don't think I see how this shows that the session yields?
Hi Ryan, Would appreciate if you could take another look. What do you think of these changes. thanks raman https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right): https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4511: // yielding. On 2013/01/19 00:32:13, Ryan Hamilton wrote: > How does this test verify that the session yields? It definitely verifies that > trans->Read() returns synchronously, but I'm not sure that's the same thing. With out DeterminsticSocketData changes and TaskObserver changes, checked if "SpdySession::DoRead" posted a task or not. This method has only one task that is getting posted. In SyncRead case, this method won't post any task. In NonSyceRead case where it yields, it posts a task. Our changes to DeterministicSocketData show that all reads are happening in SYNCHRONOUS mode. What do you think of this change. Many many thanks for all your help. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4523: net::TestDataStream test_stream; On 2013/01/19 00:32:13, Ryan Hamilton wrote: > nit: no need for test::, I think. (And in the next test) Made all these changes in SpdySession unit tests. Done. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4526: char* payload_data = payload->data(); On 2013/01/19 00:32:13, Ryan Hamilton wrote: > nit: move this line above the previous, and use payload_data instead of > payload->data() as the first argument for GetBytes(). (And in the next test). Made all these changes in SpdySession unit tests. Done. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4576: rv = trans->Read(buf, kPayloadSize, read_callback.callback()); On 2013/01/19 00:32:13, Ryan Hamilton wrote: > nit: I would just do ASSERT_LE(0, rv); Deleted this test. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4589: MessageLoop::current()->RunUntilIdle(); On 2013/01/19 00:32:13, Ryan Hamilton wrote: > Can you do this as the last thing line in this test? Deleted this test. https://codereview.chromium.org/11644088/diff/125006/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_spdy3_unittest.cc:4600: // then reads more data after yielding. On 2013/01/19 00:32:13, Ryan Hamilton wrote: > I don't think I see how this shows that the session yields? Wrote the new unit tests.
Looks reasonable. https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:202: bool posted_; I think I might be inclined to make this a count instead of a bool. https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1689: reads[i] = CreateMockRead(*data_frame, i + 1, SYNCHRONOUS); Any reason not to define the reads array with the normal: MockReads reads[] = { ... }; pattern? https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1724: SpdySessionTaskObserver observer("DoRead"); Only "DoRead"? is there a way to check for the SpdySession part too?
hi rch@, Made all the changes you have suggested. PTAL. thanks. https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:202: bool posted_; On 2013/02/05 05:03:45, Ryan Hamilton wrote: > I think I might be inclined to make this a count instead of a bool. Done. https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1689: reads[i] = CreateMockRead(*data_frame, i + 1, SYNCHRONOUS); On 2013/02/05 05:03:45, Ryan Hamilton wrote: > Any reason not to define the reads array with the normal: > > MockReads reads[] = { ... }; > > pattern? Done. https://codereview.chromium.org/11644088/diff/166011/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1724: SpdySessionTaskObserver observer("DoRead"); On 2013/02/05 05:03:45, Ryan Hamilton wrote: > Only "DoRead"? is there a way to check for the SpdySession part too? Done.
lgtm sleevi: do you want to take a look?
https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:255: bytes_read_(0), I'm still not sure I understand why we need this, since we could use a stack variable. Yes, 4-8 bytes on the heap is probably cheap enough not to worry about, but it looks so out of place for me, since it's only needed in the context of a given iteration of DoLoop. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:840: read_pending_ = true; Why is this necessary? Returning result here should cause the loop to terminate, as per line 816. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:849: DCHECK(!read_pending_); This seems wrong - you set read_pending_ to true on line 840 on ERR_IO_PENDING, with the callback bound to this function (line 838). This means that if connection_->socket()->Read() were to return ERR_IO_PENDING, and then call your callback, you should end up crashing. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:861: bytes_read_ += bytes_read; naming: having bytes_read_ (instance var) and bytes_read (scoped var) is very subtle and confusing. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.h File net/spdy/spdy_session.h (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.h#... net/spdy/spdy_session.h:48: const int kMaxReadBytes = 32 * 1024; I'm not sure I understand why these constants get exposed via the header. They're implementation details, are they not? Is this only for unit tests? https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1630: TEST_F(SpdySessionSpdy2Test, SyncRead) { Nit: You should have a high-level comment for the test indicating what is being tested, along with possible highlights of comments within the test. If this test were to fail, would it be obvious from this code what was the reason (for the test or the failure?) Having taken a few weeks from reading this code, I'm not sure I could put it together without really digging through things again (meaning I'm having trouble reviewing :-p) https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1648: framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_FIN)); Is it legal to dupe IOBuffers like this? Just wondering why the testing infrastructure doesn't enforce uniqueness here. It's (probably) fine, I just want to make sure we don't cause any weird ownership issues. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1707: TEST_F(SpdySessionSpdy2Test, ASyncRead) { Naming: AsyncRead if it's asynchronous. It looks like you're saying "A Sync[hronous] Read" https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... File net/spdy/spdy_session_test_util.h (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:16: class SpdySessionTestTaskObserver : public MessageLoop::TaskObserver { Please provide comments about what this class does. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:18: SpdySessionTestTaskObserver(const std::string& file_name, Please provide comments on what these parameters are for. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:25: virtual void DidProcessTask(const base::PendingTask& pending_task) OVERRIDE; style: Within net/ (and in general, within Chrome code), the style has largely been: 1) No whitespace between the overrides 2) A comment that explains which interface is being implemented (eg: // MessageLoop::TaskObserver implementation) This is because the style guide required this up until pkasting's grand style purge in Sept ( https://sites.google.com/a/chromium.org/dev/system/app/pages/admin/compare?wu... ). https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:27: uint32 posted_count() const { return posted_count_; } Please provide comments explaining what this number signifies.
Hi sleevi, Added comments. Deleted read_pending_. IMO, with this CL, we don't need this protection against recursion. What do you think? https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc File net/spdy/spdy_session.cc (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:255: bytes_read_(0), On 2013/02/06 23:04:46, Ryan Sleevi wrote: > I'm still not sure I understand why we need this, since we could use a stack > variable. Yes, 4-8 bytes on the heap is probably cheap enough not to worry > about, but it looks so out of place for me, since it's only needed in the > context of a given iteration of DoLoop. As we had gone over the code, bytes_read_ is used in DoLoop (initializes to 0), DoRead (posts task if it goes over kMaxReadBytes) and DoReadComplete (keeps increasing it as it reads bytes). As we have discussed it, will leave this as is. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:840: read_pending_ = true; On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Why is this necessary? Returning result here should cause the loop to terminate, > as per line 816. Deleted read_pending_. This variable seems to indicate if there is a pending read or not. It seems to have been added to prevent recursion previously. IMO, we don't need this. Whenever we call DoLoop, we are initializing read_pending_ to false. What do you think of this change? Do you see any problems with that change? https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:849: DCHECK(!read_pending_); On 2013/02/06 23:04:46, Ryan Sleevi wrote: > This seems wrong - you set read_pending_ to true on line 840 on ERR_IO_PENDING, > with the callback bound to this function (line 838). > > This means that if connection_->socket()->Read() were to return ERR_IO_PENDING, > and then call your callback, you should end up crashing. SpdySession::OnReadComplete initializes read_pending_ to false and calls DoLoop which calls DoReadComplete. As we had talked, will leave DCHECK as is. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.cc... net/spdy/spdy_session.cc:861: bytes_read_ += bytes_read; On 2013/02/06 23:04:46, Ryan Sleevi wrote: > naming: having bytes_read_ (instance var) and bytes_read (scoped var) is very > subtle and confusing. Changed "bytes_read" to "result" Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.h File net/spdy/spdy_session.h (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session.h#... net/spdy/spdy_session.h:48: const int kMaxReadBytes = 32 * 1024; On 2013/02/06 23:04:46, Ryan Sleevi wrote: > I'm not sure I understand why these constants get exposed via the header. > They're implementation details, are they not? Is this only for unit tests? This is for unit tests. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1630: TEST_F(SpdySessionSpdy2Test, SyncRead) { On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Nit: You should have a high-level comment for the test indicating what is being > tested, along with possible highlights of comments within the test. > > If this test were to fail, would it be obvious from this code what was the > reason (for the test or the failure?) Having taken a few weeks from reading this > code, I'm not sure I could put it together without really digging through things > again (meaning I'm having trouble reviewing :-p) +1 to the above. Added comments on what the tests are trying to test. Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1648: framer.CreateDataFrame(1, payload_data, kPayloadSize, DATA_FLAG_FIN)); On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Is it legal to dupe IOBuffers like this? Just wondering why the testing > infrastructure doesn't enforce uniqueness here. > > It's (probably) fine, I just want to make sure we don't cause any weird > ownership issues. SpdyFramer::CreateDataFrame calls SpdyFrameBuilder to allocate a buffer of size greater than kPayloadSize and copies the payload_data into that buffer. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1707: TEST_F(SpdySessionSpdy2Test, ASyncRead) { On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Naming: AsyncRead if it's asynchronous. It looks like you're saying "A > Sync[hronous] Read" Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... File net/spdy/spdy_session_test_util.h (right): https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:16: class SpdySessionTestTaskObserver : public MessageLoop::TaskObserver { On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Please provide comments about what this class does. Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:18: SpdySessionTestTaskObserver(const std::string& file_name, On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Please provide comments on what these parameters are for. Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:25: virtual void DidProcessTask(const base::PendingTask& pending_task) OVERRIDE; On 2013/02/06 23:04:46, Ryan Sleevi wrote: > style: Within net/ (and in general, within Chrome code), the style has largely > been: > > 1) No whitespace between the overrides > 2) A comment that explains which interface is being implemented (eg: // > MessageLoop::TaskObserver implementation) > > This is because the style guide required this up until pkasting's grand style > purge in Sept ( > https://sites.google.com/a/chromium.org/dev/system/app/pages/admin/compare?wu... > ). Done. https://codereview.chromium.org/11644088/diff/197001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:27: uint32 posted_count() const { return posted_count_; } On 2013/02/06 23:04:46, Ryan Sleevi wrote: > Please provide comments explaining what this number signifies. Done.
Mostly LGTM, but some comments below. If the follow-up test I'm requesting requires big changes to DeterministicSocketData, let's split that into a separate CL so it's easier to review. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1653: framer.CreateDataFrame(1, payload_data, kPayloadSize - 1, DATA_FLAG_FIN)); naming: data_frame1 and data_frame seem to have interesting/odd naming (one numbered, the other not - you would expect data_frame1/data_frame2, for ex) But perhaps we can improve this further: data_frame1 -> partial_data_frame data_frame -> nearly_full_data_frame These same comments apply to the following test. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1754: CreateMockRead(*data_frame, 6, SYNCHRONOUS), So one thing you're missing from only these two tests are frames where the underlying socket has more data than kMaxReadBytes (which will be a common case) Seems like you should have another test to handle say, 4 reads of kMaxReadBytes / 4 (-spdy size), one read of kMaxReadBytes (-spdysize), and perhaps some reads of kMaxReadBytes + [some value] (-spdysize), and then check that your assumptions are correct regarding the post task behaviours. I would expect that it would never try to read > 32K from the underlying transport. I'm not sure if DeterministicSocketData supports setting up MockReads that are GREATER than what the client asks for, but it does strike me as a bit of code that's untested here. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... File net/spdy/spdy_session_test_util.h (right): https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:19: // DoRead) are completed. // SpdySessionTestTaskObserver is a MessageLoop::TaskObserver // that monitors the completion of all tasks executed by the // current MessageLoop, recording the number of tasks that // refer to a specific function and filename. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:24: SpdySessionTestTaskObserver(const std::string& file_name, // Creates a SpdySessionTaskObserver that will record all // tasks that are executed that were posted by the function // named by |function_name|, located in the file |file_name|. // // Example: // file_name = "src/net/base/foo.cc" // function = "DoFoo" https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:33: // |file_name| have been completed. // Returns the number of tasks posted by the given function and file. Note: posted_count() makes it seem like you're observing the will-execute count (posted) - but you're not, you're measuring the did-execute count. Perhaps rename it as "executed_count"? Additionally, Chromium style ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... ) has a preference for integers for things that will 'not be too big, e.g., loop counters'. While not a loop counter, this does seem to apply here.
Thanks very much sleevi for your comments. Added a TODO for DeterministicSocketData changes (wrote the test you had suggested. Will enable that test after DeterministicSocketData changes). https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... File net/spdy/spdy_session_spdy2_unittest.cc (right): https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1653: framer.CreateDataFrame(1, payload_data, kPayloadSize - 1, DATA_FLAG_FIN)); On 2013/02/07 02:43:50, Ryan Sleevi wrote: > naming: data_frame1 and data_frame seem to have interesting/odd naming (one > numbered, the other not - you would expect data_frame1/data_frame2, for ex) > > But perhaps we can improve this further: > data_frame1 -> partial_data_frame > data_frame -> nearly_full_data_frame > > These same comments apply to the following test. Done. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_sp... net/spdy/spdy_session_spdy2_unittest.cc:1754: CreateMockRead(*data_frame, 6, SYNCHRONOUS), On 2013/02/07 02:43:50, Ryan Sleevi wrote: > So one thing you're missing from only these two tests are frames where the > underlying socket has more data than kMaxReadBytes (which will be a common case) > > Seems like you should have another test to handle say, 4 reads of kMaxReadBytes > / 4 (-spdy size), one read of kMaxReadBytes (-spdysize), and perhaps some reads > of kMaxReadBytes + [some value] (-spdysize), and then check that your > assumptions are correct regarding the post task behaviours. > > I would expect that it would never try to read > 32K from the underlying > transport. > > I'm not sure if DeterministicSocketData supports setting up MockReads that are > GREATER than what the client asks for, but it does strike me as a bit of code > that's untested here. Created a test that is DISABLED. Added a TODO. Will submit a separate CL after making changes to DeterministicSocketData . https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... File net/spdy/spdy_session_test_util.h (right): https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:19: // DoRead) are completed. On 2013/02/07 02:43:50, Ryan Sleevi wrote: > // SpdySessionTestTaskObserver is a MessageLoop::TaskObserver > // that monitors the completion of all tasks executed by the > // current MessageLoop, recording the number of tasks that > // refer to a specific function and filename. Done. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:24: SpdySessionTestTaskObserver(const std::string& file_name, On 2013/02/07 02:43:50, Ryan Sleevi wrote: > // Creates a SpdySessionTaskObserver that will record all > // tasks that are executed that were posted by the function > // named by |function_name|, located in the file |file_name|. > // > // Example: > // file_name = "src/net/base/foo.cc" > // function = "DoFoo" Done. https://codereview.chromium.org/11644088/diff/204001/net/spdy/spdy_session_te... net/spdy/spdy_session_test_util.h:33: // |file_name| have been completed. On 2013/02/07 02:43:50, Ryan Sleevi wrote: > // Returns the number of tasks posted by the given function and file. > > Note: posted_count() makes it seem like you're observing the will-execute count > (posted) - but you're not, you're measuring the did-execute count. Perhaps > rename it as "executed_count"? > > Additionally, Chromium style ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Intege... > ) has a preference for integers for things that will 'not be too big, e.g., loop > counters'. While not a loop counter, this does seem to apply here. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/11644088/200004
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/11644088/200004
Message was sent while issue was closed.
Change committed as 181390 |
