New issue
Advanced search Search tips

Issue 621905 link

Starred by 1 user
Status: Fixed
Owner:
b...@chromium.org
Closed: Jul 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , All
Pri: 3
Type: Bug



Sign in to add a comment

Avoid copy of SpdyHeaderBlock.

Project Member Reported by b...@chromium.org, Jun 21 2016 
SpdyHeaderBlock is somewhat expesive to copy, but most copies can be avoided.  SpdyFrameWithHeaderBlockIR constructor already takes SpdyHeaderBlock by value and moves it into header_block_ member.  In order to actually reduce copies, SpdyHeaderBlock instances should be moved around (std::move at call site and argument by value instead of const ref) along paths that take ownership.  Also, SpdyHeaderBlock instances can now be moved from heap to stack, which is a minor performance improvement but also makes code more readable by getting rid of all unique_ptrs.

Note that this is a different approach than the one contemplated in  issue 525137 .
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 23 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d

commit 2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d
Author: bnc <bnc@chromium.org>
Date: Thu Jun 23 23:08:34 2016

Reduce SpdyHeaderBlock copies with move semantics in shared code.

In order to reduce SpdyHeaderBlock copies,
* take SpdyHeaderBlock by value instead of const ref,
* std::move SpdyHeaderBlock at call sites if possible.

This lands server side change 125673600 by bnc.

BUG= 621905 

Review-Url: https://codereview.chromium.org/2093553004
Cr-Commit-Position: refs/heads/master@{#401749}

[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/quic/quic_chromium_client_stream.cc
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/quic/quic_chromium_client_stream.h
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/quic/quic_spdy_stream.cc
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/quic/quic_spdy_stream.h
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/quic/quic_spdy_stream_test.cc
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/tools/quic/quic_simple_server_stream.cc
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/tools/quic/quic_simple_server_stream.h
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/tools/quic/quic_spdy_client_stream.cc
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/tools/quic/quic_spdy_client_stream.h
[modify] https://crrev.com/2dc1cc43f5fe0e63c0e632008f4d9a8f122f5f6d/net/tools/quic/test_tools/quic_test_client.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 24 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/086b39e1a81d29610ecc1fbb851c934ae7f9db07

commit 086b39e1a81d29610ecc1fbb851c934ae7f9db07
Author: bnc <bnc@chromium.org>
Date: Fri Jun 24 13:05:26 2016

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 

Review-Url: https://codereview.chromium.org/2096713002
Cr-Commit-Position: refs/heads/master@{#401854}

[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/http/http_proxy_client_socket_pool_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/bidirectional_stream_quic_impl.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/quic_http_stream.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/quic_http_stream_test.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/quic_network_transaction_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/quic_stream_factory_test.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/test_tools/quic_test_packet_maker.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/quic/test_tools/quic_test_packet_maker.h
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/buffered_spdy_framer.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/buffered_spdy_framer.h
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/buffered_spdy_framer_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_proxy_client_socket_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_session.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_session.h
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_session_pool_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_stream_unittest.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/tools/flip_server/spdy_interface.cc
[modify] https://crrev.com/086b39e1a81d29610ecc1fbb851c934ae7f9db07/net/tools/quic/quic_simple_client.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94893a702549bf5d2e1f7c3cfafc904206f1cb57

commit 94893a702549bf5d2e1f7c3cfafc904206f1cb57
Author: bnc <bnc@chromium.org>
Date: Thu Jun 30 13:45:25 2016

Make SpdyHeaderBlock non-copyable.

Also change all remaining invocations of the copy constructor or copy assignment
operator to move or explicit copy.

This CL lands server change 126070406 by bnc.

BUG=488484,  621905 

Review-Url: https://codereview.chromium.org/2102253003
Cr-Commit-Position: refs/heads/master@{#403165}

[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/components/cronet/ios/test/quic_test_server.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/bidirectional_stream_quic_impl_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_chromium_client_stream.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_chromium_client_stream.h
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_chromium_client_stream_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_client_promised_info.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_client_promised_info_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_headers_stream_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_http_stream_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_network_transaction_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_session_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/quic_spdy_stream_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/quic/test_tools/quic_test_utils.h
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/bidirectional_stream_spdy_impl_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/buffered_spdy_framer_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/hpack/hpack_decoder_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_framer_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_header_block.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_header_block.h
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_header_block_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_protocol.h
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_stream.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_stream_test_util.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/spdy/spdy_stream_unittest.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/flip_server/spdy_interface.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/flip_server/spdy_interface_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/end_to_end_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_in_memory_cache.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_in_memory_cache.h
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_in_memory_cache_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_simple_server_session.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_simple_server_session_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_simple_server_stream.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/quic_simple_server_stream_test.cc
[modify] https://crrev.com/94893a702549bf5d2e1f7c3cfafc904206f1cb57/net/tools/quic/test_tools/quic_test_client.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c

commit 1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c
Author: bnc <bnc@chromium.org>
Date: Wed Jul 20 16:51:55 2016

Move some SpdyHeaderBlock instances from heap to stack.

I do not intend any functional change with this CL, just to reduce memory
allocations and code complexity.

* Change SpdyStream::SendRequestHeaders() argument type
  and SpdyStream::request_headers_ member type
  from unique_ptr<SpdyHeaderBlock> to SpdyHeaderBlock.
* Add bool request_headers_valid_ to replace unique_ptr nullptr test.
* Remove undefined SpdyStream::ProduceHeaderFrame() declaration.
* Change a number of local SpdyHeaderBlock variables from heap (unique_ptr) to
  stack to reduce the number of memory allocations.  Most of these are in tests,
  and one in SpdyHttpStream::SendRequest().

BUG= 621905 

Review-Url: https://codereview.chromium.org/2144933005
Cr-Commit-Position: refs/heads/master@{#406586}

[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_http_stream.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_proxy_client_socket.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_session_pool_unittest.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_stream.cc
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_stream.h
[modify] https://crrev.com/1d1d8646f16b16f04ef4b8a22ba119a7a5f4773c/net/spdy/spdy_stream_unittest.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 22 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1

commit da23393bc3adbd89f2ed5b57c1ad59def33c3ef1
Author: bnc <bnc@chromium.org>
Date: Fri Jul 22 03:58:48 2016

Eliminate SpdyHeaderBlock copy in SpdyStream.

Eliminate one SpdyHeaderBlock copy in SpdyStream::ProduceSynStreamFrame() (soon
to be renamed ProduceHeaderFrame).  This is achived by saving the GURL as soon
as request_headers_ are set.

Note that |url_| is not necessarily the same as url_from_header_block_, see, for
example, SpdyHttpStreamTest.SpdyURLTest.

Also remove test-only SpdyStream::HasUrlFromHeaders().

BUG= 621905 

Review-Url: https://codereview.chromium.org/2169583002
Cr-Commit-Position: refs/heads/master@{#407055}

[modify] https://crrev.com/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1/net/spdy/spdy_session_pool_unittest.cc
[modify] https://crrev.com/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1/net/spdy/spdy_stream.cc
[modify] https://crrev.com/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1/net/spdy/spdy_stream.h
[modify] https://crrev.com/da23393bc3adbd89f2ed5b57c1ad59def33c3ef1/net/spdy/spdy_stream_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 25 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22

commit b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22
Author: bnc <bnc@chromium.org>
Date: Mon Jul 25 20:03:43 2016

Avoid SpdyHeaderBlock copy in SpdyStream::OnPushPromiseHeadersReceived().

* Change HeaderCoalescer::headers() to release_headers() passing ownership of
  SpdyHeaderBlock.
* Change BufferedSpdyFramerVisitorInterface::OnHeaders() and OnPushPromise() to
  take SpdyHeaderBlock by value, allowing for move.
* Plumb move through SpdySession::TryCreatePushStream().
* Avoid SpdyHeaderBlock copy in SpdyStream::OnPushPromiseHeadersReceived().

BUG= 621905 

Review-Url: https://codereview.chromium.org/2174943002
Cr-Commit-Position: refs/heads/master@{#407554}

[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/buffered_spdy_framer.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/buffered_spdy_framer.h
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/buffered_spdy_framer_unittest.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/header_coalescer.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/header_coalescer.h
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_session.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_session.h
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_stream.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_stream.h
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_stream_unittest.cc
[modify] https://crrev.com/b8eba6a3e2e46a9d3d4c7be931b04d0a18f71d22/net/spdy/spdy_test_util_common.cc

Comment 7 by b...@chromium.org, Jul 26 2016

Status: Fixed (was: Started)
I believe copying SpdyHeaderBlock is now eliminated wherever it could be done without unreasonable complexity.
►

Sign in to add a comment