Emit session-level WINDOW_UPDATEs less frequently.

* Fix a bug in SpdySession::IncreaseRecvWindowSize() that was causing
  unnecessarily frequent WINDOW_UPDATEs to be sent.
* Update SpdyNetworkTransactionTest.WindowUpdateSent to test for this fix and
  verify locally that it fails without the fix.
* Add spdy_session_max_recv_window_size member and plumb it through
  HttpNetworkSession::Params, SpdySessionPool, and SpdySession.
* Rename stream_initial_recv_window_size to stream_max_recv_window_size members
  and corresponding methods.
* Set default values in HttpNetworkSession instead of SpdySession, have two
  separate constants instead of one.
* Set this member in SpdySessionDependencies to specification-prescribed default
  initial window size for tests.
* Use this default to simplify SpdyNetworkTransactionTest.SettingsPlayback and
  SpdySessionTest.SendInitialDataOnNewSession tests.
* Add SpdySessionPoolPeer methods to modify session and stream level maximum
  window size.

BUG=474208
Committed: https://crrev.com/8f0f3b63b8fd45d72ab957bdc12c714899199a72
Cr-Commit-Position: refs/heads/master@{#324184}

[Bence, 2015-04-06 16:06:10]

bnc@chromium.org changed reviewers:
+ rch@chromium.org

[Bence, 2015-04-06 16:06:10]

Ryan: PTAL.  Thanks.

[Ryan Hamilton, 2015-04-06 16:27:06]

This change looks good, but I think it should be tested.  Check out: 

  SpdyNetworkTransactionTest.SettingsPlayback

which does:

  SpdySessionPool* spdy_session_pool = helper.session()->spdy_session_pool();
  SpdySessionPoolPeer pool_peer(spdy_session_pool);
  pool_peer.SetEnableSendingInitialData(true);

I think that's what you want, right?

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc#ol...
net/spdy/spdy_session.cc:3194: GetInitialWindowSize(protocol_) / 2) {
I'm curious why this didn't work. It *looks* like it should work, but clearly
didn't :>

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:1131: int32 session_max_recv_window_size_;
Can you add a comment to this variable?

[Bence, 2015-04-07 12:32:16]

Ryan, PTAL.  Thanks for your suggestion, I was able to modify an existing test.

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc#ol...
net/spdy/spdy_session.cc:3194: GetInitialWindowSize(protocol_) / 2) {
On 2015/04/06 16:27:05, Ryan Hamilton wrote:
> I'm curious why this didn't work. It *looks* like it should work, but clearly
> didn't :>

Good question.  So GetInitialWindowSize is 64 kB, while
session_max_recv_window_size_ is 10 MB.  So what this bug means is that every
time the window size goes below 10 MB - 32 kB, a WINDOW_UPDATE is sent to bump
it up to 10 MB.  Instead we only want it when the window size goes below 5 MB. 
Also see bug description.

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:1131: int32 session_max_recv_window_size_;
On 2015/04/06 16:27:05, Ryan Hamilton wrote:
> Can you add a comment to this variable?

Done.

[Ryan Hamilton, 2015-04-07 14:56:40]

This is great! Thanks for augmenting the test. it's much improved.  A couple of
suggestions which came up because the code is clearer now.

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (left):

https://codereview.chromium.org/1061853002/diff/1/net/spdy/spdy_session.cc#ol...
net/spdy/spdy_session.cc:3194: GetInitialWindowSize(protocol_) / 2) {
On 2015/04/07 12:32:16, Bence wrote:
> On 2015/04/06 16:27:05, Ryan Hamilton wrote:
> > I'm curious why this didn't work. It *looks* like it should work, but
clearly
> > didn't :>
> 
> Good question.  So GetInitialWindowSize is 64 kB, while
> session_max_recv_window_size_ is 10 MB.  So what this bug means is that every
> time the window size goes below 10 MB - 32 kB, a WINDOW_UPDATE is sent to bump
> it up to 10 MB.  Instead we only want it when the window size goes below 5 MB.

> Also see bug description.

Oh, that method was return the protocol default, not the Chrome initial window
size? Gotcha. I read GetInitialWindowSize and expected that to return 10MB, but
I get it now. Thanks.

https://codereview.chromium.org/1061853002/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1061853002/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:78: spdy_stream_initial_recv_window_size(0),
instead of setting these to 0 by default, an alternative would be to set them to
the value we actually want them to have by default. Namely, 10MB. Then you could
remove the "foo == 0 ? foo : kSomething" checks from SpdySession.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (left):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:4419:
SpdySession::GetInitialWindowSize(GetParam().protocol)));
Can you explain why this changed? Since this test calls:

pool_peer.SetEnableSendingInitialData(true);

(Just like it did before) why is it not sending the WINDOW_UPDATE (like before)?

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6140:
TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
Boy, this test is *much* better now!

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6143: const int32
session_max_recv_window_size = 132 * 1024;
If you want this to be, more than 2x the default, then how about setting this to
be, say 2 * kDefault + 1?  (or 10*kDefault)

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6146: const int32
stream_initial_recv_window_size = 100 * 1024;
Ditto.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6151: const size_t kTargetSize =
72 * 1024;
How 'bout something like stream_size + session_size;

(I dislike magic numbers :>)

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6172:
spdy_util_.ConstructSpdyWindowUpdate(1, 52 * 1024));
more magic numbers make this test hard to reason about.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6214:
pool_peer.SetStreamInitialRecvWindowSize(stream_initial_recv_window_size);
I think this highlights a mismatch in vocabulary here. Both of these methods are
saying "buffer no more than X bytes", one for the session and on for the stream.
As a result, I would expect the names of the method to differ simply by
s/Stream/Session/. However, one uses Max and the other use Initial. I think this
should be fixed. In particular, I like your choice of Max for the new method and
recommend you use that for the Stream method (and member).  Look at this code in
SpdyStream which decides if it's time to send a window update:

  if (unacked_recv_window_bytes_ >
      session_->stream_initial_recv_window_size() / 2) {

The idea it's trying to convey is "is more than half of my max window size (aka
buffer) unacked", but it says unacked. If we had a more dynamic auto-tuned flow
control system (which we're going towards in QUIC and will likely go toward in
SPDY) the *initial* value may well be very different from the "current max" so
using max would also set us up better for the future.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1130: // Number of octets that are consumed and thus not
stored in a buffer owned by
s/octets/bytes/

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1132: // been sent yet.  Zero unless session flow
control is turned on.
Can you explain where these bytes are living and how they will eventually be
accounted for? (The SpdyIOBuffer destructor calls back to the SpdySession when
the caller is finished with them. So you can probably paraphrase that)

[Bence, 2015-04-07 20:21:38]

Ryan: PTAL.  Feels like we are getting in a good shape...

https://codereview.chromium.org/1061853002/diff/20001/net/http/http_network_s...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1061853002/diff/20001/net/http/http_network_s...
net/http/http_network_session.cc:78: spdy_stream_initial_recv_window_size(0),
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> instead of setting these to 0 by default, an alternative would be to set them
to
> the value we actually want them to have by default. Namely, 10MB. Then you
could
> remove the "foo == 0 ? foo : kSomething" checks from SpdySession.  

Done.  Nice.

I moved the constant to this file, since it's only used here.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (left):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:4419:
SpdySession::GetInitialWindowSize(GetParam().protocol)));
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> Can you explain why this changed? Since this test calls:
> 
> pool_peer.SetEnableSendingInitialData(true);
> 
> (Just like it did before) why is it not sending the WINDOW_UPDATE (like
before)?

Absolutely.  Before this change, the session level max window size was baked in
spdy_session.cc.  Now SpdySession gets it from its constructor.  In production
code, it's set to 10 MB by HttpNetworkSession.  In tests, it originates from
NormalSpdyTransactionHelper, which sets it to
GetInitialWindowSize(GetParam().protocol()), so there is no initial window
update.  This is better now because this initial window update is not relevant
to most tests, and in those for which it is, there are  SpdySessionPoolPeer
methods.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6140:
TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> Boy, this test is *much* better now!

Yup, I feel the same way.  Thanks for your guidance.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6143: const int32
session_max_recv_window_size = 132 * 1024;
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> If you want this to be, more than 2x the default, then how about setting this
to
> be, say 2 * kDefault + 1?  (or 10*kDefault)

Sorry, there are many requirements here that necessitate magic numbers.  For
example, kDefault is 64 kB for SPDY/3.1 but 64 kB - 1 B for HTTP/2, one is
divisible by 4 kB (the chunk size), the other isn't.  I chose more round numbers
and put in a bunch of asserts to explain, I think this is the best I can do.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6146: const int32
stream_initial_recv_window_size = 100 * 1024;
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> Ditto.

Done.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6151: const size_t kTargetSize =
72 * 1024;
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> How 'bout something like stream_size + session_size;
> 
> (I dislike magic numbers :>)

Done.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6172:
spdy_util_.ConstructSpdyWindowUpdate(1, 52 * 1024));
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> more magic numbers make this test hard to reason about.

Done.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_network_t...
net/spdy/spdy_network_transaction_unittest.cc:6214:
pool_peer.SetStreamInitialRecvWindowSize(stream_initial_recv_window_size);
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> I think this highlights a mismatch in vocabulary here. Both of these methods
are
> saying "buffer no more than X bytes", one for the session and on for the
stream.
> As a result, I would expect the names of the method to differ simply by
> s/Stream/Session/. However, one uses Max and the other use Initial. I think
this
> should be fixed. In particular, I like your choice of Max for the new method
and
> recommend you use that for the Stream method (and member).  Look at this code
in
> SpdyStream which decides if it's time to send a window update:
> 
>   if (unacked_recv_window_bytes_ >
>       session_->stream_initial_recv_window_size() / 2) {
> 
> The idea it's trying to convey is "is more than half of my max window size
(aka
> buffer) unacked", but it says unacked. If we had a more dynamic auto-tuned
flow
> control system (which we're going towards in QUIC and will likely go toward in
> SPDY) the *initial* value may well be very different from the "current max" so
> using max would also set us up better for the future.

Great idea.  I am changing this method and every relevant member.  Thanks.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1130: // Number of octets that are consumed and thus not
stored in a buffer owned by
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> s/octets/bytes/

Done.

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1132: // been sent yet.  Zero unless session flow
control is turned on.
On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> Can you explain where these bytes are living and how they will eventually be
> accounted for? (The SpdyIOBuffer destructor calls back to the SpdySession when
> the caller is finished with them. So you can probably paraphrase that)

Done.

[Ryan Hamilton, 2015-04-08 03:51:10]

Looks GREAT! Thanks for doing this. I think this is a huge win for readability.

LGTM

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1061853002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:1132: // been sent yet.  Zero unless session flow
control is turned on.
On 2015/04/07 20:21:38, Bence wrote:
> On 2015/04/07 14:56:40, Ryan Hamilton wrote:
> > Can you explain where these bytes are living and how they will eventually be
> > accounted for? (The SpdyIOBuffer destructor calls back to the SpdySession
when
> > the caller is finished with them. So you can probably paraphrase that)
> 
> Done.

Nice! Looks great.

[Ryan Hamilton, 2015-04-08 03:51:25]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2015-04-08 03:52:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061853002/40001

[commit-bot: I haz the power, 2015-04-08 04:37:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-08 04:38:34]

Patchset 3 (id:??) landed as
https://crrev.com/8f0f3b63b8fd45d72ab957bdc12c714899199a72
Cr-Commit-Position: refs/heads/master@{#324184}

[Bence, 2015-04-08 11:17:27]

Thanks for your help.