Avoid creating MemoryAllocatorDump for individual sockets

Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant 
socket information so that we can avoid creating individual 
MemoryAllocatorDump for each socket. MemoryAllocatorDump is expensive 
to create. This CL makes net/ MemoryDumpProvider create fewer 
MemoryAllocatorDump so that it can be enabled in Slow Reports.

BUG=669108
Review-Url: https://codereview.chromium.org/2623803002
Cr-Commit-Position: refs/heads/master@{#443103}
Committed: https://chromium.googlesource.com/chromium/src/+/998d24774d5062f8d032fedbe8870b23ad16c519

[xunjieli, 2017-01-10 02:31:38]

Description was changed from

==========
Avoid creating separate MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant socket
information so that we can avoid creating individual MemoryAllocatorDump for
each socket. MemoryAllocatorDump is expensive to create. This CL makes net/
MemoryDumpProvider create fewer MemoryAllocatorDump so that it can be enabled
in SlowReports.

BUG=669108
==========

to

==========
Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant socket
information so that we can avoid creating individual MemoryAllocatorDump for
each socket. MemoryAllocatorDump is expensive to create. This CL makes net/
MemoryDumpProvider create fewer MemoryAllocatorDump so that it can be enabled
in SlowReports.

BUG=669108
==========

[xunjieli, 2017-01-10 18:41:16]

Description was changed from

==========
Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant socket
information so that we can avoid creating individual MemoryAllocatorDump for
each socket. MemoryAllocatorDump is expensive to create. This CL makes net/
MemoryDumpProvider create fewer MemoryAllocatorDump so that it can be enabled
in SlowReports.

BUG=669108
==========

to

==========
Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant 
socket information so that we can avoid creating individual 
MemoryAllocatorDump for each socket. MemoryAllocatorDump is expensive 
to create. This CL makes net/ MemoryDumpProvider create fewer 
MemoryAllocatorDump so that it can be enabled in Slow Reports.

BUG=669108
==========

[xunjieli, 2017-01-10 21:08:57]

Patchset #1 (id:1) has been deleted

[xunjieli, 2017-01-10 21:13:57]

xunjieli@chromium.org changed reviewers:
+ eroman@chromium.org, ssid@chromium.org

[xunjieli, 2017-01-10 21:13:58]

Eric: could you take a look?
Ssid@ told me that creating a MemoryAllocatorDump is not cheap and we should
create as few dumps as possible. Currently because we are creating one dump per
socket, Slow Reports can't handle that many dumps.
This CL skip creating a memory allocator dump for each socket and report
aggregated results.

Thanks!

[xunjieli, 2017-01-10 21:16:00]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 21:16:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 22:14:17]

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

[commit-bot: I haz the power, 2017-01-10 22:14:18]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ssid, 2017-01-10 23:48:13]

Thanks for working on this. lgtm.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:34: // Number of certs used this Socket.
used by/in

https://codereview.chromium.org/2623803002/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2623803002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:557: // Dumps memory allocation stats to |stats|. Set
|*is_session_active| to
Set/Sets. consistent with Dumps

[xunjieli, 2017-01-11 02:10:14]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 02:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-11 02:19:59]

nice cleanup!
lgtm after these comments

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:713: StreamSocket::SocketMemoryStats
aggregated_stats;
Were you intending to use |aggregated_stats| in place of {total_size,
buffer_size, cert_count, serialized_cert_size} ?

Because it looks like that variable is unused right now.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:717: if (!socket_pool_dump) {
I think it would make sense to extract this outside of the loop and keep all of
the code that creates and writes to socket_pool_dump there.

How about:

if (socket_count > 0) {
  base::trace_event::MemoryAllocatorDump* socket_pool_dump =
pmd->CreateAllocatorDump(...);
  socket_pool_dump->AddScalar(...)
   ...
}

https://codereview.chromium.org/2623803002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:817: stats->cert_count =
server_cert_chain_->size();
What is the expected contract for |stats| -- assignment or increment?

Here we assign, whereas above it just increments |serialized_cert_size| (without
first initializing it).

Either approach is fine so long as we are consistent throughout.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:30: // Estimated total memory usage of this Socket.
It is worth mentioning somewhere that all of the "sizes" are in bytes.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:132: // Dumps memory allocation stats int |stats|.
Default implementation does
Please clarify whether |stats| can be assumed as being default-initialized upon
entry.
And whether implementations are explained to overwrite values or increment them.
From the implementation it looks like the expected pattern is assignment (not
increment).

[commit-bot: I haz the power, 2017-01-11 03:33:10]

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

[commit-bot: I haz the power, 2017-01-11 03:33:11]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[xunjieli, 2017-01-11 20:05:08]

Thanks a lot for the reviews!

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:713: StreamSocket::SocketMemoryStats
aggregated_stats;
On 2017/01/11 02:19:58, eroman (slow) wrote:
> Were you intending to use |aggregated_stats| in place of {total_size,
> buffer_size, cert_count, serialized_cert_size} ?
> 
> Because it looks like that variable is unused right now.

Done. Good catch! sorry about that.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:717: if (!socket_pool_dump) {
On 2017/01/11 02:19:58, eroman (slow) wrote:
> I think it would make sense to extract this outside of the loop and keep all
of
> the code that creates and writes to socket_pool_dump there.
> 
> How about:
> 
> if (socket_count > 0) {
>   base::trace_event::MemoryAllocatorDump* socket_pool_dump =
> pmd->CreateAllocatorDump(...);
>   socket_pool_dump->AddScalar(...)
>    ...
> }

Done. Thanks!

https://codereview.chromium.org/2623803002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:817: stats->cert_count =
server_cert_chain_->size();
On 2017/01/11 02:19:58, eroman (slow) wrote:
> What is the expected contract for |stats| -- assignment or increment?
> 
> Here we assign, whereas above it just increments |serialized_cert_size|
(without
> first initializing it).
> 
> Either approach is fine so long as we are consistent throughout.

Done. Opted for the assignment way throughout.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:30: // Estimated total memory usage of this Socket.
On 2017/01/11 02:19:58, eroman (slow) wrote:
> It is worth mentioning somewhere that all of the "sizes" are in bytes.

Done.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:34: // Number of certs used this Socket.
On 2017/01/10 23:48:13, ssid wrote:
> used by/in

Done.

https://codereview.chromium.org/2623803002/diff/20001/net/socket/stream_socke...
net/socket/stream_socket.h:132: // Dumps memory allocation stats int |stats|.
Default implementation does
On 2017/01/11 02:19:58, eroman (slow) wrote:
> Please clarify whether |stats| can be assumed as being default-initialized
upon
> entry.
> And whether implementations are explained to overwrite values or increment
them.
> From the implementation it looks like the expected pattern is assignment (not
> increment).

Done.

https://codereview.chromium.org/2623803002/diff/20001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/2623803002/diff/20001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:557: // Dumps memory allocation stats to |stats|. Set
|*is_session_active| to
On 2017/01/10 23:48:13, ssid wrote:
> Set/Sets. consistent with Dumps

Done.

[xunjieli, 2017-01-11 20:05:12]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-01-11 20:05:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2623803002/#ps80001
(title: "Add NET_EXPORT_PRIVATE to fix WIN compile")

[commit-bot: I haz the power, 2017-01-11 20:05:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 20:23:57]

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

[commit-bot: I haz the power, 2017-01-11 20:23:58]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[xunjieli, 2017-01-11 21:54:55]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-01-11 21:55:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-11 22:15:14]

lgtm

https://codereview.chromium.org/2623803002/diff/80001/net/socket/client_socke...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/client_socke...
net/socket/client_socket_handle.h:127: // |stats| and does not increment them.
same comment here ("does not increment" is probably unnecessary)

https://codereview.chromium.org/2623803002/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:814: size_t serialized_cert_size = 0;
This change shouldn't matter since the contract is |status| is default
initialized upon entry right?

I suggest removing this local variable and keeping the code as before
(incrementing |stats|, and optionally initialize here). My comment before was
really just about documenting the assumption that |status| is
default-initialized when entered here).

https://codereview.chromium.org/2623803002/diff/80001/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/stream_socke...
net/socket/stream_socket.h:134: // |stats| and not increment them. Default
implementation does nothing.
nit: can omit the phrasing about "not increment them". If it is default
initialized, then incrementing has the same effect as overwriting (assuming you
start at 0).

[xunjieli, 2017-01-11 22:16:40]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2017-01-11 23:19:25]

Thanks!

https://codereview.chromium.org/2623803002/diff/80001/net/socket/client_socke...
File net/socket/client_socket_handle.h (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/client_socke...
net/socket/client_socket_handle.h:127: // |stats| and does not increment them.
On 2017/01/11 22:15:14, eroman (slow) wrote:
> same comment here ("does not increment" is probably unnecessary)

Done.

https://codereview.chromium.org/2623803002/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_impl.cc:814: size_t serialized_cert_size = 0;
On 2017/01/11 22:15:14, eroman (slow) wrote:
> This change shouldn't matter since the contract is |status| is default
> initialized upon entry right?
> 
> I suggest removing this local variable and keeping the code as before
> (incrementing |stats|, and optionally initialize here). My comment before was
> really just about documenting the assumption that |status| is
> default-initialized when entered here).

Done. Got it, thanks!

https://codereview.chromium.org/2623803002/diff/80001/net/socket/stream_socket.h
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2623803002/diff/80001/net/socket/stream_socke...
net/socket/stream_socket.h:134: // |stats| and not increment them. Default
implementation does nothing.
On 2017/01/11 22:15:14, eroman (slow) wrote:
> nit: can omit the phrasing about "not increment them". If it is default
> initialized, then incrementing has the same effect as overwriting (assuming
you
> start at 0).

Done.

[xunjieli, 2017-01-11 23:19:38]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-01-11 23:19:38]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2623803002/#ps100001
(title: "Address Eric's comments")

[commit-bot: I haz the power, 2017-01-11 23:21:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 01:12:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484176778585470,
"parent_rev": "aafe57504428af825ec9aaf4f690194bb504acbe", "commit_rev":
"998d24774d5062f8d032fedbe8870b23ad16c519"}

[commit-bot: I haz the power, 2017-01-12 01:13:26]

Description was changed from

==========
Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant 
socket information so that we can avoid creating individual 
MemoryAllocatorDump for each socket. MemoryAllocatorDump is expensive 
to create. This CL makes net/ MemoryDumpProvider create fewer 
MemoryAllocatorDump so that it can be enabled in Slow Reports.

BUG=669108
==========

to

==========
Avoid creating MemoryAllocatorDump for individual sockets

This CL creates a StreamSocket::SocketMemoryStats to hold relevant 
socket information so that we can avoid creating individual 
MemoryAllocatorDump for each socket. MemoryAllocatorDump is expensive 
to create. This CL makes net/ MemoryDumpProvider create fewer 
MemoryAllocatorDump so that it can be enabled in Slow Reports.

BUG=669108

Review-Url: https://codereview.chromium.org/2623803002
Cr-Commit-Position: refs/heads/master@{#443103}
Committed:
https://chromium.googlesource.com/chromium/src/+/998d24774d5062f8d032fedbe887...
==========

[commit-bot: I haz the power, 2017-01-12 01:13:27]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/998d24774d5062f8d032fedbe887...