Reduce SpdyHeaderBlock copies with move semantics.

Reduce SpdyHeaderBlock copies with move semantics.

In order to reduce SpdyHeaderBlock copies and to move SpdyHeaderBlock instances
from heap to stack,
* take SpdyHeaderBlock by value instead of const ref or unique_ptr,
* std::move SpdyHeaderBlock at call sites if possible,
* return SpdyHeaderBlock by value instead of unique_ptr.

BUG=621905
Committed: https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07
Cr-Commit-Position: refs/heads/master@{#401854}

[Bence, 2016-06-23 17:22:09]

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

[Bence, 2016-06-23 17:22:10]

Ryan: PTAL.  Thank you.

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

lgtm

woot

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.c...
net/quic/quic_http_stream.cc:687: request_headers_ = SpdyHeaderBlock();
Doesn't move do the equivalent of clear()? maybe not? In any case, why assign to
a new object instead of clear()?

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

https://codereview.chromium.org/2096713002/diff/1/net/spdy/spdy_session.cc#ol...
net/spdy/spdy_session.cc:1100: // TODO(hkhalil): Avoid copy of |block|.
yay!

[Bence, 2016-06-23 23:12:25]

Patchset #2 (id:20001) has been deleted

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

Ryan: does my reasoning to remove request_headers_.clear() sound correct?

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.c...
net/quic/quic_http_stream.cc:687: request_headers_ = SpdyHeaderBlock();
On 2016/06/23 18:23:18, Ryan Hamilton wrote:
> Doesn't move do the equivalent of clear()? maybe not? In any case, why assign
to
> a new object instead of clear()?

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:08]

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.c...
net/quic/quic_http_stream.cc:687: request_headers_ = SpdyHeaderBlock();
On 2016/06/23 23:36:16, Bence wrote:
> On 2016/06/23 18:23:18, Ryan Hamilton wrote:
> > Doesn't move do the equivalent of clear()? maybe not? In any case, why
assign
> to
> > a new object instead of clear()?
> 
> 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.

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:10]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-06-24 11:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2096713002/#ps80001
(title: "Revert removing |request_headers_.clear()|.")

[commit-bot: I haz the power, 2016-06-24 11:01:24]

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

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

Thank you.

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/2096713002/diff/1/net/quic/quic_http_stream.c...
net/quic/quic_http_stream.cc:687: request_headers_ = SpdyHeaderBlock();
On 2016/06/24 00:24:07, Ryan Hamilton wrote:
> On 2016/06/23 23:36:16, Bence wrote:
> > On 2016/06/23 18:23:18, Ryan Hamilton wrote:
> > > Doesn't move do the equivalent of clear()? maybe not? In any case, why
> assign
> > to
> > > a new object instead of clear()?
> > 
> > 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.
> 
> 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.

Done.

[commit-bot: I haz the power, 2016-06-24 11:25:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-24 11:25:21]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Bence, 2016-06-24 12:08:36]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-06-24 12:08:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/2096713002/#ps100001
(title: "clear() does not work after all.")

[commit-bot: I haz the power, 2016-06-24 12:08:50]

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

[commit-bot: I haz the power, 2016-06-24 13:05:42]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-24 13:07:19]

Description was changed from

==========
Reduce SpdyHeaderBlock copies with move semantics.

In order to reduce SpdyHeaderBlock copies and to move SpdyHeaderBlock instances
from heap to stack,
* take SpdyHeaderBlock by value instead of const ref or unique_ptr,
* std::move SpdyHeaderBlock at call sites if possible,
* return SpdyHeaderBlock by value instead of unique_ptr.

BUG=621905
==========

to

==========
Reduce SpdyHeaderBlock copies with move semantics.

In order to reduce SpdyHeaderBlock copies and to move SpdyHeaderBlock instances
from heap to stack,
* take SpdyHeaderBlock by value instead of const ref or unique_ptr,
* std::move SpdyHeaderBlock at call sites if possible,
* return SpdyHeaderBlock by value instead of unique_ptr.

BUG=621905

Committed: https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07
Cr-Commit-Position: refs/heads/master@{#401854}
==========

[commit-bot: I haz the power, 2016-06-24 13:07:20]

Patchset 5 (id:??) landed as
https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07
Cr-Commit-Position: refs/heads/master@{#401854}

[Bence, 2016-06-24 13:13:10]

I needed to copy from a newly created SpdyHeaderBlock after all, because clear()
crashes on a moved-from instance.  This is a bug in SpdyHeaderBlock
implementation that I will fix in a separate CL.  Craziness!