Reduce SpdyHeaderBlock copies with move semantics.

net/quic/quic_http_stream.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_http_stream.h"
 
+#include <utility>
+
 #include "base/callback_helpers.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 #include "net/quic/quic_client_promised_info.h"
 #include "net/quic/quic_http_utils.h"
@@ skipping 654 matching lines @@
     return ERR_UNEXPECTED;
 
   // Log the actual request with the URL Request's net log.
   stream_net_log_.AddEvent(
       NetLog::TYPE_HTTP_TRANSACTION_QUIC_SEND_REQUEST_HEADERS,
       base::Bind(&QuicRequestNetLogCallback, stream_->id(), &request_headers_,
                  priority_));
   bool has_upload_data = request_body_stream_ != nullptr;
 
   next_state_ = STATE_SEND_HEADERS_COMPLETE;
-  size_t frame_len =
-      stream_->WriteHeaders(request_headers_, !has_upload_data, nullptr);
+  size_t frame_len = stream_->WriteHeaders(std::move(request_headers_),
+                                           !has_upload_data, nullptr);
   headers_bytes_sent_ += frame_len;
 
-  request_headers_.clear();
+  request_headers_ = SpdyHeaderBlock();

[Ryan Hamilton, 2016/06/23 18:23:18]

Doesn't move do the equivalent of clear()? maybe not? In any case, why assign to
a new object instead of clear()?

[Bence, 2016/06/23 23:36:16]

I believe the convention is that the moved-from object is left in valid but
unspecified state, so if this class had relied on |request_headers_| being empty
after this, then move would not have been sufficient.

However, I do not think clear() was necessary here in the first place.  The only
methods reading |request_headers_| are HandlePromise (called by SendRequest) and
DoSendHeaders, and I believe neither of them can be called by the state machine
after this point.  So I am removing this line here, just like you proposed,
though for a different reason.

[Ryan Hamilton, 2016/06/24 00:24:07]

Ah! Right. So clear() in this case was to ensure that the headers were no longer
consuming memory in the QuicHttpStream. So if the convention is that we don't
know what state request_headers_ is in, then it seems like we should clear() (or
copy from empty as your patch did) to be safe.

[Bence, 2016/06/24 11:01:42]

Done.

   return static_cast<int>(frame_len);
 }
 
 int QuicHttpStream::DoSendHeadersComplete(int rv) {
   if (rv < 0)
     return rv;
 
   // If the stream is already closed, don't read the request the body.
   if (!stream_)
     return response_status_;
@@ skipping 121 matching lines @@
   closed_stream_sent_bytes_ = stream_->stream_bytes_written();
   stream_ = nullptr;
 
   // If |request_body_stream_| is non-NULL, Reset it, to abort any in progress
   // read.
   if (request_body_stream_)
     request_body_stream_->Reset();
 }
 
 }  // namespace net