Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

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

[xunjieli, 2017-02-01 21:16:28]

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

[commit-bot: I haz the power, 2017-02-01 21:16:48]

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

[xunjieli, 2017-02-01 21:34:01]

Patchset #1 (id:1) has been deleted

[xunjieli, 2017-02-01 22:08:49]

Description was changed from

==========
Improve memory estimate of SpdySession in net/ MemoryDumpProvider.

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

to

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.
However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

[xunjieli, 2017-02-01 22:13:42]

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

[xunjieli, 2017-02-01 22:13:43]

Bence: With the upstream code in the mix, I am not sure if I can do something
like this. Let's talk in person tomorrow. Thanks!

[xunjieli, 2017-02-01 22:14:12]

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

[commit-bot: I haz the power, 2017-02-01 22:14:34]

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-02-01 23:23:49]

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

[commit-bot: I haz the power, 2017-02-01 23:23:50]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-02-02 14:15:19]

LGTM with nits and questions.

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server.cc
File net/proxy/proxy_server.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server....
net/proxy/proxy_server.cc:213: return
HostPortPair::EstimateMemoryUsage(host_port_pair_);
1.It's just a string, how bad can it get?  Few dozen bytes?
2. Why do we not account for memory used by enum Scheme (maybe 64 bits depending
on memory alignment requirements)?
3. Why does HostPortPair::EstimateMemoryUsage() not account for uint16 port,
only the string?

I do not mean to be nitpicking, but since there are multiple kilobyte buffers
and containers hanging around, it feels arbitrary to me to account for few dozen
byte long strings, but not go all the way and account for ints.

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/hpack/hpack_st...
File net/spdy/hpack/hpack_static_table.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/hpack/hpack_st...
net/spdy/hpack/hpack_static_table.cc:43: // TODO(xunjieli): return the
allocation of |static_entries_| when std::deque
Please add reference to https://crbug.com/669108.

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1085: // TODO(xunjieli): crbug.com/669108. Track
|pending_create_stream_queue_|,
Nit: https://crbug.com/669108.  For the convenience of developers who use tools
that make a URL clickable only if scheme is present.

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_write_que...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_write_que...
net/spdy/spdy_write_queue.cc:160: // memory_usage_estimator.h supports
std::deque. crbug.com/669108.
Nit: https://crbug.com/669108.

[xunjieli, 2017-02-02 14:47:26]

Description was changed from

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.
However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

to

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

[xunjieli, 2017-02-02 14:53:41]

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

[xunjieli, 2017-02-02 14:53:42]

Dmitry and Sid: PTAL
Is there a bug tracking the TODO of adding std::deque support in
memory_usage_estimator.h?

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server.cc
File net/proxy/proxy_server.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server....
net/proxy/proxy_server.cc:213: return
HostPortPair::EstimateMemoryUsage(host_port_pair_);
On 2017/02/02 14:15:19, Bence wrote:
> 1.It's just a string, how bad can it get?  Few dozen bytes?
> 2. Why do we not account for memory used by enum Scheme (maybe 64 bits
depending
> on memory alignment requirements)?
> 3. Why does HostPortPair::EstimateMemoryUsage() not account for uint16 port,
> only the string?
> 
> I do not mean to be nitpicking, but since there are multiple kilobyte buffers
> and containers hanging around, it feels arbitrary to me to account for few
dozen
> byte long strings, but not go all the way and account for ints.

dskiba@ can correct me if I am wrong. My understanding is that EMU() should only
account for dynamically allocated memory. For members(such as enum and ints)
that do not allocate, we don't track them. The static allocated memory is
accounted for in a tree Node.
More discussion on this topic can be found in a review comment here:
https://codereview.chromium.org/2650943007/diff/100001/net/base/host_port_pai...

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/hpack/hpack_st...
File net/spdy/hpack/hpack_static_table.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/hpack/hpack_st...
net/spdy/hpack/hpack_static_table.cc:43: // TODO(xunjieli): return the
allocation of |static_entries_| when std::deque
On 2017/02/02 14:15:19, Bence wrote:
> Please add reference to https://crbug.com/669108.

Done.

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1085: // TODO(xunjieli): crbug.com/669108. Track
|pending_create_stream_queue_|,
On 2017/02/02 14:15:19, Bence wrote:
> Nit: https://crbug.com/669108.  For the convenience of developers who use
tools
> that make a URL clickable only if scheme is present.

Done.

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_write_que...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/spdy/spdy_write_que...
net/spdy/spdy_write_queue.cc:160: // memory_usage_estimator.h supports
std::deque. crbug.com/669108.
On 2017/02/02 14:15:19, Bence wrote:
> Nit: https://crbug.com/669108.

Done.

[Bence, 2017-02-02 15:05:09]

Thank you.  Still LGTM.

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server.cc
File net/proxy/proxy_server.cc (right):

https://codereview.chromium.org/2665283003/diff/40001/net/proxy/proxy_server....
net/proxy/proxy_server.cc:213: return
HostPortPair::EstimateMemoryUsage(host_port_pair_);
Ah okay, that makes perfect sense.  Thank you for the explanation.

[DmitrySkiba, 2017-02-02 18:16:27]

Overall comments:

1. I'll implement EMU for std::deque (and friends) ASAP. If you can hold this
for couple of days, that would be great.

2. We have several scoped_refptrs too. Eventually I'll implement EMU for it too,
but for now let's just mark such members with TODOs. (We should do that for all
other cases too.)

3. I would prefer that we call EMU(x) even if x has EMU() member function. We
have a special global overload for EMU(x) that just returns x.EMU() if member
function is available.

4. EMU() implementation should be just list of EMU() invocation on members, it
shouldn't have any conditional flows. EMUs that take pointers or smart pointers
deal with nulls by just returning 0.

5. It looks like in several places in net code already has memory estimations of
some sort (size(), bytes_allocated()), it would be interesting to see if we can
migrate them to EMUs.

6. Watch out for code that uses std::vector/string.size() as memory usage
estimation - it should use capacity() instead (or just call EMU).

https://codereview.chromium.org/2665283003/diff/60001/net/http2/hpack/decoder...
File net/http2/hpack/decoder/hpack_whole_entry_buffer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/http2/hpack/decoder...
net/http2/hpack/decoder/hpack_whole_entry_buffer.cc:36: return
name_.BufferedLength() + value_.BufferedLength();
Hmm, can we add EMU to HpackDecoderStringBuffer? BufferLength() returns length
of the string it contains, not its capacity.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/buffered_spdy_...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/buffered_spdy_...
net/spdy/buffered_spdy_framer.cc:341:
base::trace_event::EstimateMemoryUsage(coalescer_);
I would also add control_frame_fields_ and  goaway_fields_.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_de...
File net/spdy/hpack/hpack_decoder2.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_de...
net/spdy/hpack/hpack_decoder2.cc:164: return header_table_.size() +
HpackHeaderTable::size() seems to be total size of the entries themselves, and
doesn't include overhead of deque and unordered_map. I think we need EMU for
that.

Also, same comment regarding BufferLength() - I think HpackDecoderStringBuffer
should just implement EMU().

BTW, can HpackEntry::Size() use EMUs too?

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_en...
File net/spdy/hpack/hpack_encoder.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_en...
net/spdy/hpack/hpack_encoder.cc:137: return header_table_.size() +
output_stream_.size();
They should both be EMUs - header_table_.size() it doesn't include overhead from
deque / unordered_map, and output_stream_.size() returns buffer's size, not
capacity.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_st...
File net/spdy/hpack/hpack_static_table.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_st...
net/spdy/hpack/hpack_static_table.cc:45: return 0;
This class also has static_index_ and static_name_index_, which should be
estimated too.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_buffer.cc
File net/spdy/spdy_buffer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_buffer.cc...
net/spdy/spdy_buffer.cc:102: return shared_frame_->data->size();
I think that SpdySerializedFrame should implement EMU itself, because it knows
whether it owns the buffer.

Also please add TODO about estimating consume_callbacks_ once base::Callback
supports EMU.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_framer.cc...
net/spdy/spdy_framer.cc:2423: current_frame_buffer_.len() +
settings_scratch_.buffer.len();
CharBuffer should implement EMU (where it should return capacity, not length),
that would also make estimating altsvc_scratch_ trivial.

Please also estimate decoder_adapter_ (i.e. add EMUs to SpdyFramerDecoderAdapter
and its subclasses).

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_header_bl...
File net/spdy/spdy_header_block.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_header_bl...
net/spdy/spdy_header_block.cc:302: size_t SpdyHeaderBlock::EstimateMemoryUsage()
const {
I think block_ should also be estimated.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1073:
base::trace_event::EstimateMemoryUsage(spdy_session_key_) +
connection_, unclaimed_pushed_streams_, priority_dependency_state_ should also
be estimated.

Please leave TODO about estimating read_buffer_ (scoped_refptr).

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1080: memory_estimate +=
base::trace_event::EstimateMemoryUsage(in_flight_write_);
You don't need this if, EMU(unique_ptr) returns 0 if the pointer holds null.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session_k...
File net/spdy/spdy_session_key.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session_k...
net/spdy/spdy_session_key.cc:55: return
HostPortPair::EstimateMemoryUsage(host_port_proxy_pair_.first) +
Why EMU() is not an instance method in HostPortPair? It it was you could write
just EMU(host_port_proxy_pair_) because we have generic EMU for std::pair.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:825:
base::trace_event::EstimateMemoryUsage(request_headers_);
Please also estimate url_from_header_block_ and pending_recv_data_.

Please add TODO regarding pending_send_data_ (scoped_refptr).

[xunjieli, 2017-02-02 18:58:40]

Thanks, Dmitry! I've learned more about EMU today.
The reason that I sometimes invoke the member function EMU is that several files
here are shared with the internal repo which can't depend on
base/trace_event/memory_usage_estimator.h. I will talk with Bence tomorrow on
whether we can upstream this particular file (memory_usage_estimator.h/cc) to
use base::trace_event::EMU() throughout net/ codebase. This will also make
things easier for QUIC, which is in the similar case as Http2.

I'll hold off on this CL until std::deque is implemented. I will also sort out
the case of upstream with Bence. Please cc me on the std::deque bug/CL. Thanks!

[xunjieli, 2017-02-02 18:59:17]

Description was changed from

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

to

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

[xunjieli, 2017-02-03 22:22:43]

Description was changed from

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support std::deque.

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

to

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
scoped_refptr and WeakPtr.

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

[xunjieli, 2017-02-03 22:24:35]

Description was changed from

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
scoped_refptr and WeakPtr.

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

to

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

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

[xunjieli, 2017-02-03 22:25:10]

Thanks! I am unblocked and have uploaded a new patch. PTAL.

https://codereview.chromium.org/2665283003/diff/60001/net/http2/hpack/decoder...
File net/http2/hpack/decoder/hpack_whole_entry_buffer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/http2/hpack/decoder...
net/http2/hpack/decoder/hpack_whole_entry_buffer.cc:36: return
name_.BufferedLength() + value_.BufferedLength();
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> Hmm, can we add EMU to HpackDecoderStringBuffer? BufferLength() returns length
> of the string it contains, not its capacity. 

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/buffered_spdy_...
File net/spdy/buffered_spdy_framer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/buffered_spdy_...
net/spdy/buffered_spdy_framer.cc:341:
base::trace_event::EstimateMemoryUsage(coalescer_);
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> I would also add control_frame_fields_ and  goaway_fields_.

Done for goaway_fields_. ControlFrameFields do not have members that can
allocate, so I omitted it.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_de...
File net/spdy/hpack/hpack_decoder2.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_de...
net/spdy/hpack/hpack_decoder2.cc:164: return header_table_.size() +
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> HpackHeaderTable::size() seems to be total size of the entries themselves, and
> doesn't include overhead of deque and unordered_map. I think we need EMU for
> that.
> 
> Also, same comment regarding BufferLength() - I think HpackDecoderStringBuffer
> should just implement EMU().
> 

Done.

> BTW, can HpackEntry::Size() use EMUs too?
That's shared code. Let's keep it as it is and do a follow-up.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_en...
File net/spdy/hpack/hpack_encoder.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_en...
net/spdy/hpack/hpack_encoder.cc:137: return header_table_.size() +
output_stream_.size();
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> They should both be EMUs - header_table_.size() it doesn't include overhead
from
> deque / unordered_map, and output_stream_.size() returns buffer's size, not
> capacity.

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_st...
File net/spdy/hpack/hpack_static_table.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/hpack/hpack_st...
net/spdy/hpack/hpack_static_table.cc:45: return 0;
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> This class also has static_index_ and static_name_index_, which should be
> estimated too.

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_buffer.cc
File net/spdy/spdy_buffer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_buffer.cc...
net/spdy/spdy_buffer.cc:102: return shared_frame_->data->size();
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> I think that SpdySerializedFrame should implement EMU itself, because it knows
> whether it owns the buffer.
> 
> Also please add TODO about estimating consume_callbacks_ once base::Callback
> supports EMU.

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_framer.cc...
net/spdy/spdy_framer.cc:2423: current_frame_buffer_.len() +
settings_scratch_.buffer.len();
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> CharBuffer should implement EMU (where it should return capacity, not length),
> that would also make estimating altsvc_scratch_ trivial.
> 
> Please also estimate decoder_adapter_ (i.e. add EMUs to
SpdyFramerDecoderAdapter
> and its subclasses).
> 

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_header_bl...
File net/spdy/spdy_header_block.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_header_bl...
net/spdy/spdy_header_block.cc:302: size_t SpdyHeaderBlock::EstimateMemoryUsage()
const {
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> I think block_ should also be estimated.

I don't think EMU has support for linked_hash_map?

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1073:
base::trace_event::EstimateMemoryUsage(spdy_session_key_) +
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> connection_, unclaimed_pushed_streams_, priority_dependency_state_ should also
> be estimated.
> 
> Please leave TODO about estimating read_buffer_ (scoped_refptr).

Done. I estimated |read_buffer_| including kReadBufferSize. |connection_| is
estimated in |stats->total_size|.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:1080: memory_estimate +=
base::trace_event::EstimateMemoryUsage(in_flight_write_);
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> You don't need this if, EMU(unique_ptr) returns 0 if the pointer holds null.

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session_k...
File net/spdy/spdy_session_key.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_session_k...
net/spdy/spdy_session_key.cc:55: return
HostPortPair::EstimateMemoryUsage(host_port_proxy_pair_.first) +
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> Why EMU() is not an instance method in HostPortPair? It it was you could write
> just EMU(host_port_proxy_pair_) because we have generic EMU for std::pair.

Done.

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/2665283003/diff/60001/net/spdy/spdy_stream.cc...
net/spdy/spdy_stream.cc:825:
base::trace_event::EstimateMemoryUsage(request_headers_);
On 2017/02/02 18:16:27, DmitrySkiba wrote:
> Please also estimate url_from_header_block_ and pending_recv_data_.
> 
> Please add TODO regarding pending_send_data_ (scoped_refptr).

Done.

[xunjieli, 2017-02-03 22:26:36]

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

[xunjieli, 2017-02-03 22:26:42]

Description was changed from

==========
[ON HOLD] Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

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

to

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

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

[commit-bot: I haz the power, 2017-02-03 22:27:11]

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-02-03 22:30:45]

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

[commit-bot: I haz the power, 2017-02-03 22:30:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xunjieli, 2017-02-03 23:02:52]

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

[commit-bot: I haz the power, 2017-02-03 23:03:24]

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-02-04 00:25:43]

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

[commit-bot: I haz the power, 2017-02-04 00:25:44]

Dry run: This issue passed the CQ dry run.

[Bence, 2017-02-05 19:11:23]

LGTM with nits.

https://codereview.chromium.org/2665283003/diff/100001/net/http2/hpack/decode...
File net/http2/hpack/decoder/http2_hpack_decoder.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/http2/hpack/decode...
net/http2/hpack/decoder/http2_hpack_decoder.cc:120: return
entry_buffer_.EstimateMemoryUsage();
I thought EstimateMemoryUsage(entry_buffer_) is preferred per
https://codereview.chromium.org/2665283003/#msg16 comment 3.

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/header_coales...
File net/spdy/header_coalescer.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/header_coales...
net/spdy/header_coalescer.cc:74: return headers_.EstimateMemoryUsage();
Use EstimateMemoryUsage(headers_) instead?

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/hpack/hpack_d...
File net/spdy/hpack/hpack_decoder3.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/hpack/hpack_d...
net/spdy/hpack/hpack_decoder3.cc:111: return
hpack_decoder_.EstimateMemoryUsage();
EstimateMemoryUsage(hpack_decoder_)

[xunjieli, 2017-02-06 14:53:32]

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

[commit-bot: I haz the power, 2017-02-06 14:53:46]

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

[xunjieli, 2017-02-06 14:54:56]

Thanks! Dmitry and Sid: PTAL.

https://codereview.chromium.org/2665283003/diff/100001/net/http2/hpack/decode...
File net/http2/hpack/decoder/http2_hpack_decoder.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/http2/hpack/decode...
net/http2/hpack/decoder/http2_hpack_decoder.cc:120: return
entry_buffer_.EstimateMemoryUsage();
On 2017/02/05 19:11:23, Bence wrote:
> I thought EstimateMemoryUsage(entry_buffer_) is preferred per
> https://codereview.chromium.org/2665283003/#msg16 comment 3.

Done. Oops sorry. Good catch! I missed this one and the ones below.

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/header_coales...
File net/spdy/header_coalescer.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/header_coales...
net/spdy/header_coalescer.cc:74: return headers_.EstimateMemoryUsage();
On 2017/02/05 19:11:23, Bence wrote:
> Use EstimateMemoryUsage(headers_) instead?

Done.

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/hpack/hpack_d...
File net/spdy/hpack/hpack_decoder3.cc (right):

https://codereview.chromium.org/2665283003/diff/100001/net/spdy/hpack/hpack_d...
net/spdy/hpack/hpack_decoder3.cc:111: return
hpack_decoder_.EstimateMemoryUsage();
On 2017/02/05 19:11:23, Bence wrote:
> EstimateMemoryUsage(hpack_decoder_)

Done.

[commit-bot: I haz the power, 2017-02-06 16:28:45]

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

[commit-bot: I haz the power, 2017-02-06 16:28:46]

Dry run: This issue passed the CQ dry run.

[DmitrySkiba, 2017-02-06 18:02:34]

Almost there! Thanks for working on this!

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/hpack/hpack_d...
File net/spdy/hpack/hpack_decoder.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/hpack/hpack_d...
net/spdy/hpack/hpack_decoder.cc:121: return header_table_.size() +
HpackHeaderTable has EMU now, so this should be EMU(header_table_).

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/http2_priorit...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/http2_priorit...
net/spdy/http2_priority_dependencies.cc:164: // when memory_usage_estimator.h
supports std::list::iterator.
Nice catch! I'll implement EMUs for iterators (I would expect iterators to have
trivial dtors, but I guess some STL implementations have debug checks in them).

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_write_qu...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_write_qu...
net/spdy/spdy_write_queue.cc:161: return 0;
std::deque is supported now, so this should be EMU(queue_).

[Bence, 2017-02-08 14:46:46]

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_protocol...
net/spdy/spdy_protocol.h:863: if (owns_buffer_)
Optional: if you think it is easier to read, maybe write
return owns_buffer_ ? size_ : 0;

[xunjieli, 2017-02-08 15:42:36]

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

[xunjieli, 2017-02-08 15:42:50]

Thanks! PTAL

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/hpack/hpack_d...
File net/spdy/hpack/hpack_decoder.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/hpack/hpack_d...
net/spdy/hpack/hpack_decoder.cc:121: return header_table_.size() +
On 2017/02/06 18:02:34, DmitrySkiba wrote:
> HpackHeaderTable has EMU now, so this should be EMU(header_table_).

Done.

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/http2_priorit...
File net/spdy/http2_priority_dependencies.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/http2_priorit...
net/spdy/http2_priority_dependencies.cc:164: // when memory_usage_estimator.h
supports std::list::iterator.
On 2017/02/06 18:02:34, DmitrySkiba wrote:
> Nice catch! I'll implement EMUs for iterators (I would expect iterators to
have
> trivial dtors, but I guess some STL implementations have debug checks in
them).

Acknowledged. Thanks! I will file 690015 to keep track of it. Updated the
comment as well.

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_protocol...
net/spdy/spdy_protocol.h:863: if (owns_buffer_)
On 2017/02/08 14:46:45, Bence wrote:
> Optional: if you think it is easier to read, maybe write
> return owns_buffer_ ? size_ : 0;

Done.

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_write_qu...
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/2665283003/diff/120001/net/spdy/spdy_write_qu...
net/spdy/spdy_write_queue.cc:161: return 0;
On 2017/02/06 18:02:34, DmitrySkiba wrote:
> std::deque is supported now, so this should be EMU(queue_).

Done.

[commit-bot: I haz the power, 2017-02-08 15:43:06]

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

[DmitrySkiba, 2017-02-08 16:38:23]

LGTM!

[commit-bot: I haz the power, 2017-02-08 17:49:17]

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

[commit-bot: I haz the power, 2017-02-08 17:49:18]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-02-09 20:40:10]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-09 20:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org,
dskiba@chromium.org
Link to the patchset: https://codereview.chromium.org/2665283003/#ps200001
(title: "rebased")

[commit-bot: I haz the power, 2017-02-09 20:40:53]

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-02-09 22:04:20]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486672810648980,
"parent_rev": "1c04025f853bc892cb4064cc5c7711bd33facbd6", "commit_rev":
"ec84e4ea604deb4d6cca4b72ce6f97babf99878b"}

[commit-bot: I haz the power, 2017-02-09 22:04:58]

Description was changed from

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

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

to

==========
Improve memory estimate of SpdySessionPool in net/ MemoryDumpProvider.

This CL instruments other objects (e.g. buffers, queues, strings) 
referenced from SpdySessionPool and SpdySession. The goal is
to make the memory estimate reported by 
SpdySessionPool::DumpMemoryStats() cover more parts of the code.

However, there are still a couple of TODOs because 
base/trace_event/memory_usage_estimator.h doesn't yet support 
linked_hash_map, scoped_refptr and WeakPtr.

BUG=669108

Review-Url: https://codereview.chromium.org/2665283003
Cr-Commit-Position: refs/heads/master@{#449431}
Committed:
https://chromium.googlesource.com/chromium/src/+/ec84e4ea604deb4d6cca4b72ce6f...
==========

[commit-bot: I haz the power, 2017-02-09 22:05:00]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ec84e4ea604deb4d6cca4b72ce6f...