Emit session-level WINDOW_UPDATEs less frequently.

net/spdy/spdy_network_transaction_unittest.cc

Index: net/spdy/spdy_network_transaction_unittest.cc
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index d4f023f3de18e901792f553959ee999864057e36..07dac07e3c4f8f71c847f84d603c677421f532f4 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -4411,13 +4411,6 @@ TEST_P(SpdyNetworkTransactionTest, SettingsPlayback) {
   scoped_ptr<SpdyFrame> initial_settings_frame(
       spdy_util_.ConstructSpdySettings(initial_settings));
 
-  // Construct the initial window update.
-  scoped_ptr<SpdyFrame> initial_window_update(
-      spdy_util_.ConstructSpdyWindowUpdate(
-          kSessionFlowControlStreamId,
-          kDefaultInitialRecvWindowSize -
-              SpdySession::GetInitialWindowSize(GetParam().protocol)));

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

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)?

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

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.

-
   // Construct the persisted SETTINGS frame.
   const SettingsMap& settings =
       spdy_session_pool->http_server_properties()->GetSpdySettings(
@@ -4438,9 +4431,6 @@ TEST_P(SpdyNetworkTransactionTest, SettingsPlayback) {
                   kHttp2ConnectionHeaderPrefixSize));
   }
   writes.push_back(CreateMockWrite(*initial_settings_frame));
-  if (GetParam().protocol >= kProtoSPDY31) {
-    writes.push_back(CreateMockWrite(*initial_window_update));
-  }
   writes.push_back(CreateMockWrite(*settings_frame));
   writes.push_back(CreateMockWrite(*req));
 
@@ -6148,23 +6138,50 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateReceived) {
 // Test that received data frames and sent WINDOW_UPDATE frames change
 // the recv_window_size_ correctly.
 TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {

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

Boy, this test is *much* better now!

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

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

-  const int32 initial_window_size =
-      SpdySession::GetInitialWindowSize(GetParam().protocol);
-  // Amount of body required to trigger a sent window update.
-  const size_t kTargetSize = initial_window_size / 2 + 1;
+  // Session level window size that is more than twice the default 65536 or
+  // 65535 bytes so that an initial window update is sent.
+  const int32 session_max_recv_window_size = 132 * 1024;

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

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)

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

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.

+  // Stream level window size that is more than the default 65536 or 65535
+  // bytes but less than the session level window size.
+  const int32 stream_initial_recv_window_size = 100 * 1024;

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

Ditto.

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

Done.

+  // Amount of body to be sent.  Has to less than or equal to both window sizes
+  // so that we do not run out of receiving window, and greater than half of
+  // them so that it triggers both a session level and a stream level window
+  // update frame.
+  const size_t kTargetSize = 72 * 1024;

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

How 'bout something like stream_size + session_size;

(I dislike magic numbers :>)

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

Done.

 
+  SettingsMap initial_settings;
+  initial_settings[SETTINGS_MAX_CONCURRENT_STREAMS] =
+      SettingsFlagsAndValue(SETTINGS_FLAG_NONE, kMaxConcurrentPushedStreams);
+  initial_settings[SETTINGS_INITIAL_WINDOW_SIZE] = SettingsFlagsAndValue(
+      SETTINGS_FLAG_NONE, stream_initial_recv_window_size);
+  scoped_ptr<SpdyFrame> initial_settings_frame(
+      spdy_util_.ConstructSpdySettings(initial_settings));
+  scoped_ptr<SpdyFrame> initial_window_update(
+      spdy_util_.ConstructSpdyWindowUpdate(
+          kSessionFlowControlStreamId,
+          session_max_recv_window_size -
+              SpdySession::GetInitialWindowSize(GetParam().protocol)));
   scoped_ptr<SpdyFrame> req(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
+  // WINDOW_UPDATEs are sent for the smaller multiple of 4 kB exceeding half the
+  // respective windows, because DATA frames constructed below are 4 kB each.
   scoped_ptr<SpdyFrame> session_window_update(
-      spdy_util_.ConstructSpdyWindowUpdate(0, kTargetSize));
-  scoped_ptr<SpdyFrame> window_update(
-      spdy_util_.ConstructSpdyWindowUpdate(1, kTargetSize));
+      spdy_util_.ConstructSpdyWindowUpdate(0, 68 * 1024));
+  scoped_ptr<SpdyFrame> stream_window_update(
+      spdy_util_.ConstructSpdyWindowUpdate(1, 52 * 1024));

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

more magic numbers make this test hard to reason about.

[Bence, 2015/04/07 20:21:37]

Done.

 
   std::vector<MockWrite> writes;
+  if ((GetParam().protocol >= kProtoSPDY4MinimumVersion) &&
+      (GetParam().protocol <= kProtoSPDY4MaximumVersion)) {
+    writes.push_back(MockWrite(ASYNC, kHttp2ConnectionHeaderPrefix,
+                               kHttp2ConnectionHeaderPrefixSize, 0));
+  }
+  writes.push_back(CreateMockWrite(*initial_settings_frame));
+  writes.push_back(CreateMockWrite(*initial_window_update));
   writes.push_back(CreateMockWrite(*req));
-  if (GetParam().protocol >= kProtoSPDY31)
-    writes.push_back(CreateMockWrite(*session_window_update));
-  writes.push_back(CreateMockWrite(*window_update));
+  writes.push_back(CreateMockWrite(*session_window_update));
+  writes.push_back(CreateMockWrite(*stream_window_update));
 
   std::vector<MockRead> reads;
   scoped_ptr<SpdyFrame> resp(
@@ -6182,15 +6199,21 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
   }
   reads.push_back(MockRead(ASYNC, ERR_IO_PENDING, 0));  // Yield.
 
-  DelayedSocketData data(1, vector_as_array(&reads), reads.size(),
+  DelayedSocketData data(2, vector_as_array(&reads), reads.size(),
                          vector_as_array(&writes), writes.size());
 
   NormalSpdyTransactionHelper helper(CreateGetRequest(), DEFAULT_PRIORITY,
                                      BoundNetLog(), GetParam(), NULL);
   helper.AddData(&data);
   helper.RunPreTestSetup();
-  HttpNetworkTransaction* trans = helper.trans();
 
+  SpdySessionPool* spdy_session_pool = helper.session()->spdy_session_pool();
+  SpdySessionPoolPeer pool_peer(spdy_session_pool);
+  pool_peer.SetEnableSendingInitialData(true);
+  pool_peer.SetSessionMaxRecvWindowSize(session_max_recv_window_size);
+  pool_peer.SetStreamInitialRecvWindowSize(stream_initial_recv_window_size);

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

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.

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

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

+
+  HttpNetworkTransaction* trans = helper.trans();
   TestCompletionCallback callback;
   int rv = trans->Start(&helper.request(), callback.callback(), BoundNetLog());
 
@@ -6204,7 +6227,7 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
   ASSERT_TRUE(stream->stream() != NULL);
 
   // All data has been read, but not consumed. The window reflects this.
-  EXPECT_EQ(static_cast<int>(initial_window_size - kTargetSize),
+  EXPECT_EQ(static_cast<int>(stream_initial_recv_window_size - kTargetSize),
             stream->stream()->recv_window_size());
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
@@ -6218,7 +6241,7 @@ TEST_P(SpdyNetworkTransactionTest, WindowUpdateSent) {
   scoped_refptr<IOBuffer> buf(new IOBuffer(kTargetSize));
   EXPECT_EQ(static_cast<int>(kTargetSize),
             trans->Read(buf.get(), kTargetSize, CompletionCallback()));
-  EXPECT_EQ(static_cast<int>(initial_window_size),
+  EXPECT_EQ(static_cast<int>(stream_initial_recv_window_size),
             stream->stream()->recv_window_size());
   EXPECT_THAT(base::StringPiece(buf->data(), kTargetSize), Each(Eq('x')));