SpdySession: Combine three frames into a single packet.

net/spdy/chromium/spdy_session.cc

Index: net/spdy/chromium/spdy_session.cc
diff --git a/net/spdy/chromium/spdy_session.cc b/net/spdy/chromium/spdy_session.cc
index 7158a1fa84eaf68e418da894fa7fab301e07fe1d..6e0fa6dff87d9d8d60d193bc297886bb2edc6dc0 100644
--- a/net/spdy/chromium/spdy_session.cc
+++ b/net/spdy/chromium/spdy_session.cc
@@ -2123,48 +2123,83 @@ int SpdySession::DoWriteComplete(int result) {
 
 void SpdySession::SendInitialData() {
   DCHECK(enable_sending_initial_data_);
+  DCHECK(buffered_spdy_framer_.get());
 
-  auto connection_header_prefix_frame = base::MakeUnique<SpdySerializedFrame>(
-      const_cast<char*>(kHttp2ConnectionHeaderPrefix),
-      kHttp2ConnectionHeaderPrefixSize, false /* take_ownership */);
-  // Count the prefix as part of the subsequent SETTINGS frame.
-  EnqueueSessionWrite(HIGHEST, SpdyFrameType::SETTINGS,
-                      std::move(connection_header_prefix_frame));
-
-  // First, notify the server about the settings they should use when
-  // communicating with us.  Only send settings that have a value different from
-  // the protocol default value.
+  // Prepare initial SETTINGS frame.  Only send settings that have a value
+  // different from the protocol default value.
   SettingsMap settings_map;
   for (auto setting : initial_settings_) {
     if (!IsSpdySettingAtDefaultInitialValue(setting.first, setting.second)) {
       settings_map.insert(setting);
     }
   }
-  SendSettings(settings_map);
+  net_log_.AddEvent(NetLogEventType::HTTP2_SESSION_SEND_SETTINGS,
+                    base::Bind(&NetLogSpdySendSettingsCallback, &settings_map));
+  std::unique_ptr<SpdySerializedFrame> settings_frame(
+      buffered_spdy_framer_->CreateSettings(settings_map));
 
-  // Next, notify the server about our initial recv window size.
-  // Bump up the receive window size to the real initial value. This
-  // has to go here since the WINDOW_UPDATE frame sent by
-  // IncreaseRecvWindowSize() call uses |buffered_spdy_framer_|.
-  // This condition implies that |session_max_recv_window_size_| -
-  // |session_recv_window_size_| doesn't overflow.
+  // Prepare initial WINDOW_UPDATE frame.
+  // Make sure |session_max_recv_window_size_ - session_recv_window_size_|
+  // does not underflow.
   DCHECK_GE(session_max_recv_window_size_, session_recv_window_size_);
   DCHECK_GE(session_recv_window_size_, 0);
-  if (session_max_recv_window_size_ > session_recv_window_size_) {
-    IncreaseRecvWindowSize(session_max_recv_window_size_ -
-                           session_recv_window_size_);
+  std::unique_ptr<SpdySerializedFrame> window_update_frame;
+  bool send_window_update =
+      session_max_recv_window_size_ > session_recv_window_size_;
+  if (send_window_update) {
+    DCHECK_GE(session_unacked_recv_window_bytes_, 0);
+    DCHECK_GE(session_recv_window_size_, session_unacked_recv_window_bytes_);
+
+    const int32_t delta_window_size =
+        session_max_recv_window_size_ - session_recv_window_size_;
+    session_recv_window_size_ += delta_window_size;
+    net_log_.AddEvent(NetLogEventType::HTTP2_STREAM_UPDATE_RECV_WINDOW,
+                      base::Bind(&NetLogSpdySessionWindowUpdateCallback,
+                                 delta_window_size, session_recv_window_size_));
+
+    session_unacked_recv_window_bytes_ += delta_window_size;
+    if (session_unacked_recv_window_bytes_ <= session_max_recv_window_size_ / 2)
+      send_window_update = false;
+  }
+
+  if (send_window_update) {

[Zhongyi Shi, 2017/05/30 23:10:49]

nit: If I understand the code correctly, the previous block was updating recv
window only. And this block is actually send the window update. 

Instead of using |send_window_update|, could you change that to 
if (session_max_recv_window_size_ > session_recv_window_size_) {
  // Update RECV window
  ...
}
if (session_unacked_recv_window_bytes_ > session_max_recv_window_size_ / 2) {
  // Send Window update.
  ...
}

I think that's more readable and not so confusing.

[Bence, 2017/06/05 17:19:02]

I agree with you that this logic flow is unnecessary complicated, thank you for
catching it.

How about always sending a WINDOW_UPDATE frame if |session_max_recv_window_size_
> session_recv_window_size_|?  This would simplify things even further, and
would not matter in practice: |session_recv_window_size_| is defined to be
initially 64 kB in the protocol specification [1, 2], and
|session_max_recv_window_size_| is 15 MB in production [3].

[1]
https://cs.chromium.org/chromium/src/net/spdy/chromium/spdy_session.h?q=kDefa...
[2] http://httpwg.org/specs/rfc7540.html#SettingValues
[3] https://cs.chromium.org/chromium/src/net/http/http_network_session.cc?l=70

+    net_log_.AddEvent(NetLogEventType::HTTP2_SESSION_SEND_WINDOW_UPDATE,
+                      base::Bind(&NetLogSpdyWindowUpdateFrameCallback,
+                                 kSessionFlowControlStreamId,
+                                 session_unacked_recv_window_bytes_));
+    window_update_frame = buffered_spdy_framer_->CreateWindowUpdate(
+        kSessionFlowControlStreamId, session_unacked_recv_window_bytes_);
+    session_unacked_recv_window_bytes_ = 0;
   }
-}
 
-void SpdySession::SendSettings(const SettingsMap& settings) {
-  net_log_.AddEvent(NetLogEventType::HTTP2_SESSION_SEND_SETTINGS,
-                    base::Bind(&NetLogSpdySendSettingsCallback, &settings));
-  // Create the SETTINGS frame and send it.
-  DCHECK(buffered_spdy_framer_.get());
-  std::unique_ptr<SpdySerializedFrame> settings_frame(
-      buffered_spdy_framer_->CreateSettings(settings));
+  // Create a single frame to hold connection prefix, initial SETTINGS frame,
+  // and optional initial WINDOW_UPDATE frame, so that they are sent on the wire
+  // in a single packet.
+  size_t initial_frame_size =
+      kHttp2ConnectionHeaderPrefixSize + settings_frame->size();
+  if (send_window_update)
+    initial_frame_size += window_update_frame->size();
+  auto initial_frame_data = base::MakeUnique<char[]>(initial_frame_size);
+  size_t offset = 0;
+
+  memcpy(initial_frame_data.get() + offset, kHttp2ConnectionHeaderPrefix,
+         kHttp2ConnectionHeaderPrefixSize);
+  offset += kHttp2ConnectionHeaderPrefixSize;
+
+  memcpy(initial_frame_data.get() + offset, settings_frame->data(),
+         settings_frame->size());
+  offset += settings_frame->size();
+
+  if (send_window_update) {
+    memcpy(initial_frame_data.get() + offset, window_update_frame->data(),
+           window_update_frame->size());
+  }
+
+  auto initial_frame = base::MakeUnique<SpdySerializedFrame>(
+      initial_frame_data.release(), initial_frame_size,
+      /* owns_buffer = */ true);
   EnqueueSessionWrite(HIGHEST, SpdyFrameType::SETTINGS,
-                      std::move(settings_frame));
+                      std::move(initial_frame));
 }
 
 void SpdySession::HandleSetting(uint32_t id, uint32_t value) {