New issue
Advanced search Search tips

Issue 600438 link

Starred by 0 users

Issue metadata

Status: Assigned
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature



Sign in to add a comment

Move SpdySerializedFrame from heap to stack.

Project Member Reported by b...@chromium.org, Apr 4 2016

Issue description

SpdySerializedFrame is always used wrapped in a scoped_ptr<> to manage memory ownership: to make sure there is only one instance pointing to owned memory, and it always gets freed after destruction.  However, the same thing could be enforced with move semantics, which would then allow SpdySerializedFrame to be always instantiated on the stack, providing slight performance improvements, as well as more readable code.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 6 2016

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

commit b03b109170504bbf5cea0a33327574196bc21c76
Author: bnc <bnc@chromium.org>
Date: Wed Apr 06 11:19:55 2016

Implement SpdySerializedFrame move semantics.

* Rename SpdyFrame to SpdySerializedFrame everywhere.  (It was already
  called that at a number of places.)  Remove typedef.
* Add move constructor and move assignment operator.
* Change return value from SpdySerializedFrame* to SpdySerializedFrame for
  SpdyFramer methods and SpdyFrameBuilder::take().
* Remove scoped_ptr.get() == nullptr checks which could never possibly be true anyway.
* Merge SpdySerializedFrame instantiation and assignment in a few tests (except
  where lifetime requirements would not allow it.)
* Remove declaration of undefined, unused method SpdyFramer::CompressFrame().
* Remove unused variable |uncompressed| from SpdyFramerTest.Basic.
* git clang-format.

This cuts down on SpdySerializedFrame allocations, and also trigger compiler
errors for unused local variables.  (Unused scoped_ptr<>'s like |uncompressed|
did not trigger an error.)

This is a minimal CL to land server change 117967994 by bnc.  It will be
followed up by other CLs moving instances of SpdySerializedFramer from heap to
stack.

BUG=488484, 600438

Review URL: https://codereview.chromium.org/1852423004

Cr-Commit-Position: refs/heads/master@{#385436}

[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/http/http_proxy_client_socket_pool_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/quic/quic_headers_stream.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/quic/quic_headers_stream_test.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/quic/spdy_utils.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/quic/test_tools/quic_test_packet_maker.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/buffered_spdy_framer.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/buffered_spdy_framer.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/buffered_spdy_framer_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_buffer.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_buffer.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_buffer_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_frame_builder.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_frame_builder_test.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_framer.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_framer.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_framer_test.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_http_stream_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_protocol.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_proxy_client_socket_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_session.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_session.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_session_pool_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_stream.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_stream.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_stream_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_test_util_common.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_test_utils.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_test_utils.h
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/spdy/spdy_write_queue_unittest.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/tools/flip_server/spdy_interface.cc
[modify] https://crrev.com/b03b109170504bbf5cea0a33327574196bc21c76/net/tools/flip_server/spdy_interface_test.cc

Project Member

Comment 2 by sheriffbot@chromium.org, Jun 1 2016

Labels: -M-52 M-53 MovedFrom-52
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 14 2016

Labels: -M-53 MovedFrom-53
This issue has been moved once and is lower than Pri-1. Removing the milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

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

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

commit df80d44fd869b5a27871be4c5c1e0fdfd184d584
Author: bnc <bnc@chromium.org>
Date: Fri Jul 15 20:27:41 2016

Move ~1000 SpdySerializedFrame instances from heap to stack in tests.

* Return SpdySerializedFrame instead of SpdySerializedFrame* in SpdyTestUtil.
* Move ~1000 SpdySerializedFrame local variables from heap to stack in tests,
  thus eliminating ~1000 unnecessary memory allocations.
* Remove unused SpdyTestUtil::ConstructSpdyFrame() methods.
* s/SpdyTestUtil::ConstructSpdyBodyFrame/SpdyTestUtil::ConstructSpdyDataFrame/
  (Body is an application layer concept; data is a framing layer concept.)
* Use SpdyTestUtil::ConstructSpdyDataFrame() instead of
  BufferedSpdyFramer::CreateDataFrame() in tests;
  remove unused BufferedSpdyFramer instances.
* Remove one unused unique_ptr<SpdySerializedFrame> instance from
  BidirectionalStreamTest.CancelStreamDuringReadData.

This change only affects tests, there is no functional change intended, and
there is no move semantics involved.

BUG=600438

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

[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/http/bidirectional_stream_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/http/http_proxy_client_socket_pool_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/bidirectional_stream_spdy_impl_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_http_stream_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_proxy_client_socket_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_session_pool_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_session_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_stream_unittest.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_test_util_common.cc
[modify] https://crrev.com/df80d44fd869b5a27871be4c5c1e0fdfd184d584/net/spdy/spdy_test_util_common.h

Status: Assigned (was: Started)
This is marked "started", yet hasn't been updated in a long time. Changing back to assigned.
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 11 2018

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

commit dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125
Author: Bence Béky <bnc@chromium.org>
Date: Sun Feb 11 01:32:29 2018

Misc cleanup: remove unused, fix comment, improve logging.

Change logging in StaticSocketDataHelper::VerifyWriteData() such that if
writted data does not match mock write, then print actual written data in full length, not
only truncated to the length of the mock write data.

Remove unused StaticSocketDataProvider:CompleteRead().

The rest of this CL belongs to https://crbug.com/810752:

Inline ConstructSpdyGoAway() as ConstructSpdyGoAway(0).

Remove unused struct SpdyHeaderInfo.

Remove obsolete "The caller takes ownership of the frame." comment from
spdy_test_util_common.h, leftover from when methods used to return raw
pointers.  This is a follow-up to
https://crrev.com/2156643002/diff/1/net/spdy/spdy_test_util_common.h.

Also remove useless "Returns a SpdySerializedFrame." comments from the
line above the definition showing return type SpdySerializedFrame.

Bug: 600438, 810752
Change-Id: I1f3471f4e4a9b71603257cf82986d0efe0288c7f
Reviewed-on: https://chromium-review.googlesource.com/912013
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535993}
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/socket/socket_test_util.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/socket/socket_test_util.h
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/dcb30096edc00cebf7aff7ec50cdfcfc2d5e9125/net/spdy/chromium/spdy_test_util_common.h

Sign in to add a comment