Track SimpleCache memory usage in net/ MemoryDumpProvider

Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.
BlockFile and In-memory cache will be implemented in a
follow-up.

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

[xunjieli, 2017-01-31 19:47:23]

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

[commit-bot: I haz the power, 2017-01-31 19:47:54]

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

[xunjieli, 2017-01-31 20:49:08]

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

[xunjieli, 2017-01-31 20:49:09]

Gavin and Sid: PTAL. Thanks!

[commit-bot: I haz the power, 2017-01-31 20:58:49]

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

[commit-bot: I haz the power, 2017-01-31 20:58:50]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-01-31 22:24:20]

My main question here is do we have stats about how much memory other parts and
code paths use. Since this implementation very specifically targets the
EntryMetaData and overlooks everything else.

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_backend_impl.h:124: const std::string&
parent_absolute_name) const override;
Do we have stats about how often we use the blockfile or mem_backend
implementation of the Backend? Is it the case that most of the time we use
SimpleCache and most of the memory usage is from EntryMetaData and we would not
have to care about the others? If not, we should make this pure and add TODO in
the classes that says implement this later.

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_index.cc:277: return
base::trace_event::EstimateMemoryUsage(entries_set_);
Do we need a todo here for other objects here, like removed_entries_ and
index_file_?
Or is it not required if the memory usage is not significant?

https://codereview.chromium.org/2661333002/diff/1/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/1/net/http/http_cache.cc#newc...
net/http/http_cache.cc:487: disk_cache_->DumpMemoryStats(pmd,
parent_absolute_name);
Are the other members here insignificant? like active_entries_, doomed_entries_,
playback_cache_map_ and network_layer_.

[xunjieli, 2017-02-01 01:15:52]

> My main question here is do we have stats about how much memory other parts
and
> code paths use. Since this implementation very specifically targets the
> EntryMetaData and overlooks everything else.

You are absolutely right about this CL ignoring other parts of the code. Since
the memory effort is primarily focused on Android where SimpleCache is enabled
by default, this CL will track the majority of the allocation from
net/disk_cache  (~80% from what I see locally). BlockFile is being replaced. 
In-memory cache is only used for incognito (I think).  

My goal is to match MemoryDumpProvider with what I see in native heap profiling
traces. I start from top of the list and try to keep each CL small so it's
easier to review. Happy to add TODOs and comments where applicable :)

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_backend_impl.h:124: const std::string&
parent_absolute_name) const override;
On 2017/01/31 22:24:19, ssid wrote:
> Do we have stats about how often we use the blockfile or mem_backend
> implementation of the Backend? Is it the case that most of the time we use
> SimpleCache and most of the memory usage is from EntryMetaData and we would
not
> have to care about the others? If not, we should make this pure and add TODO
in
> the classes that says implement this later.

We are moving towards using SimpleCache everywhere.  It's currently being used
on Android and Linux by default.  That's what show up in traces that I obtained
from Linux and Android. I don't think we care about the others, but I will add a
TODO to investigate.

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2661333002/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_index.cc:277: return
base::trace_event::EstimateMemoryUsage(entries_set_);
On 2017/01/31 22:24:19, ssid wrote:
> Do we need a todo here for other objects here, like removed_entries_ and
> index_file_?
> Or is it not required if the memory usage is not significant?

I added |removed_entries_|. SimpleIndexFile doesn't hold on to allocations much.
It's a bunch of static functions.

https://codereview.chromium.org/2661333002/diff/1/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/1/net/http/http_cache.cc#newc...
net/http/http_cache.cc:487: disk_cache_->DumpMemoryStats(pmd,
parent_absolute_name);
On 2017/01/31 22:24:19, ssid wrote:
> Are the other members here insignificant? like active_entries_,
doomed_entries_,
> playback_cache_map_ and network_layer_.

I don't know. I haven't seen those in traces that I obtained. Let's tackle those
in a follow-up. I added TODO.

[ssid, 2017-02-01 02:45:48]

On 2017/02/01 01:15:52, xunjieli wrote:
> > My main question here is do we have stats about how much memory other parts
> and
> > code paths use. Since this implementation very specifically targets the
> > EntryMetaData and overlooks everything else.
> 
> You are absolutely right about this CL ignoring other parts of the code. Since
> the memory effort is primarily focused on Android where SimpleCache is enabled
> by default, this CL will track the majority of the allocation from
> net/disk_cache  (~80% from what I see locally). BlockFile is being replaced. 
> In-memory cache is only used for incognito (I think).  
> 
> My goal is to match MemoryDumpProvider with what I see in native heap
profiling
> traces. I start from top of the list and try to keep each CL small so it's
> easier to review. Happy to add TODOs and comments where applicable :)

Okay I think the memory instrumentation effort should be platform independent.
That is we should be giving a good estimate of memory usage on all platforms,
not just Android. So, let's not leave out memory unaccounted in Mac and Windows
which are major platforms too. We will soon have a team working on desktop
memory reduction which will face problems because of these missing pieces. 

I agree with keeping CLs small. But, lets not leave too much debt around which
might take very long to get back to. I would be happy if we have some way to
verify that we would actually cover 80% of the disk cache memory in all cases,
or there could be cases in the wild where the user has lot of active_entries_
and doomed_entries_ we did not care to account for. If you could verify that
these would never happen or that the entries use insignificant memory, you could
remove the TODO right away and ignore them. Else if it is hard to verify that
the usage is insignificant then lets keep the TODOs. Maybe gavinp@ would know
about this and could help here?

https://codereview.chromium.org/2661333002/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2661333002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:184: // SimpleCache.
Slightly weird place to have this TODO about implementation at the interface. I
would suggest making this pure virtual and having TODO in the actual classes.

https://codereview.chromium.org/2661333002/diff/20001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/20001/net/http/http_cache.cc#...
net/http/http_cache.cc:488: // TODO(xunjieli): Track other member allocations.
I think TODOs are generally followed by bug numbers in case someone else tries
to fix it in future.

[pasko, 2017-02-01 05:39:40]

On 2017/02/01 01:15:52, xunjieli wrote:
> > My main question here is do we have stats about how much memory other parts
> and
> > code paths use. Since this implementation very specifically targets the
> > EntryMetaData and overlooks everything else.
> 
> You are absolutely right about this CL ignoring other parts of the code. Since
> the memory effort is primarily focused on Android where SimpleCache is enabled
> by default, this CL will track the majority of the allocation from
> net/disk_cache  (~80% from what I see locally). BlockFile is being replaced.

I think nobody here would be surprised to hear that memory consumption is
tricky. Some web pages may peak memory in active_entries_, as the page is
loaded, the active entries would free up themselves. Other pages are not as
concurrent => less bloat in active entries. So it is important to know what
pages this conclusion is based on, and at what time the measurements are taken.

Having only some parts instrumented whole other parts not instrumented .. would
confuse people in the future, which is what I would like to avoid. If the
justification is a local experiment, please make sure to document your
experimentation methodology, provide a data file with results of your experiment
and reference both from a comment in the code (or from a bug/doc that is
referenced in the code).

As usual with experimental data, the situation may change, the bottlenecks may
shift, the memory usage patterns may drift, and we would need to re-evaluate
whether the testing methodology is still applicable in the new system. Having
reproducible testing "stories" (i.e. scenarios) would help a lot in such cases
to keep maintenance burden to minimum.

> In-memory cache is only used for incognito (I think).  

I believe this is correct.

[xunjieli, 2017-02-01 14:33:41]

Description was changed from

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider. This CL currently only
tracks EntryMetaData, which accounts for ~80% of memory used by SimpleCache.

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

to

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.


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

[xunjieli, 2017-02-01 14:55:18]

My method is simply open a couple of tabs (with a reused profile) wait for a bit
and get a memory dump. Then I will compare the estimate with that from native
heap profiler. It's a very crude method. The trick is, of course, to make the
estimate as accurate as possible that we can have meaningful data from the
field, so thank you both for the valuable feedback.

I uploaded a new patch to address the comments. PTAL.

Here's a trace file:
https://drive.google.com/open?id=0B_9WseIqRoaFUEYtR2RnbHJ2akU

As you can see in the MemoryDumpProvider view (screenshot:
https://drive.google.com/open?id=0B_9WseIqRoaFR2REdkJQN20zQ1k) , there's 375.1 +
9.6 KiB from "http_cache" This number roughly matches what we see in heap
profiler 385.3 + 5.9 KiB (screenshot:
https://drive.google.com/open?id=0B_9WseIqRoaFQnRkUDU4dkZuOGM).

https://codereview.chromium.org/2661333002/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache.h (right):

https://codereview.chromium.org/2661333002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache.h:184: // SimpleCache.
On 2017/02/01 02:45:48, ssid wrote:
> Slightly weird place to have this TODO about implementation at the interface.
I
> would suggest making this pure virtual and having TODO in the actual classes.

Done.

https://codereview.chromium.org/2661333002/diff/20001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/20001/net/http/http_cache.cc#...
net/http/http_cache.cc:488: // TODO(xunjieli): Track other member allocations.
On 2017/02/01 02:45:48, ssid wrote:
> I think TODOs are generally followed by bug numbers in case someone else tries
> to fix it in future.

Done. Removed the TODO and added tracking for other member allocations.

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

Description was changed from

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.


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

to

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.
BlockFile and In-memory cache will be implemented in a
follow-up.

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

[xunjieli, 2017-02-01 15:00:58]

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

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

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 15:19:34]

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

[commit-bot: I haz the power, 2017-02-01 15:19:35]

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

[xunjieli, 2017-02-01 16:31:45]

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

[commit-bot: I haz the power, 2017-02-01 16:31:58]

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 17:51:11]

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

[commit-bot: I haz the power, 2017-02-01 17:51:31]

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 19:19:27]

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

[commit-bot: I haz the power, 2017-02-01 19:19:28]

Dry run: This issue passed the CQ dry run.

[ssid, 2017-02-02 19:30:01]

lgtm thanks!

https://codereview.chromium.org/2661333002/diff/80001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/80001/net/http/http_cache.cc#...
net/http/http_cache.cc:128: base::trace_event::EstimateMemoryUsage(writer) +
Do we need to measure the memory used by each transaction. The Transaction
objects seems to be large? Does the SimpleBackendImpl own the Transaction
objects? Why doesn't it use unique_ptr?

[xunjieli, 2017-02-03 17:30:27]

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

[xunjieli, 2017-02-03 17:30:27]

jkarlin@: PTAL at content/browser/cache_storage/cache_storage_cache_unittest.cc

Gavin: a friendly ping! Thanks.

https://codereview.chromium.org/2661333002/diff/80001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/80001/net/http/http_cache.cc#...
net/http/http_cache.cc:128: base::trace_event::EstimateMemoryUsage(writer) +
On 2017/02/02 19:30:01, ssid wrote:
> Do we need to measure the memory used by each transaction. The Transaction
> objects seems to be large? Does the SimpleBackendImpl own the Transaction
> objects? Why doesn't it use unique_ptr?

SimpleBackendImpl doesn't own the Transaction objects. The Transaction is owned
by net::URLRequest. I have a WIP CL to tackle URLRequest and friends.

[jkarlin, 2017-02-03 17:46:13]

cache_storage/ lgtm

[xunjieli, 2017-02-03 17:51:16]

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

[commit-bot: I haz the power, 2017-02-03 17:52:00]

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 18:50:04]

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

[commit-bot: I haz the power, 2017-02-03 18:50:05]

Dry run: This issue passed the CQ dry run.

[pasko, 2017-02-03 22:39:16]

On 2017/02/01 14:55:18, xunjieli wrote:
> My method is simply open a couple of tabs (with a reused profile) wait for a
bit
> and get a memory dump. Then I will compare the estimate with that from native
> heap profiler. It's a very crude method. The trick is, of course, to make the
> estimate as accurate as possible that we can have meaningful data from the
> field, so thank you both for the valuable feedback.
> 
> I uploaded a new patch to address the comments. PTAL.
> 
> Here's a trace file:
> https://drive.google.com/open?id=0B_9WseIqRoaFUEYtR2RnbHJ2akU
> 
> As you can see in the MemoryDumpProvider view (screenshot:
> https://drive.google.com/open?id=0B_9WseIqRoaFR2REdkJQN20zQ1k) , there's 375.1
+
> 9.6 KiB from "http_cache" This number roughly matches what we see in heap
> profiler 385.3 + 5.9 KiB (screenshot:
> https://drive.google.com/open?id=0B_9WseIqRoaFQnRkUDU4dkZuOGM).

can you make these files World-readable please?

[xunjieli, 2017-02-03 22:44:40]

On 2017/02/03 22:39:16, pasko wrote:
> On 2017/02/01 14:55:18, xunjieli wrote:
> > My method is simply open a couple of tabs (with a reused profile) wait for a
> bit
> > and get a memory dump. Then I will compare the estimate with that from
native
> > heap profiler. It's a very crude method. The trick is, of course, to make
the
> > estimate as accurate as possible that we can have meaningful data from the
> > field, so thank you both for the valuable feedback.
> > 
> > I uploaded a new patch to address the comments. PTAL.
> > 
> > Here's a trace file:
> > https://drive.google.com/open?id=0B_9WseIqRoaFUEYtR2RnbHJ2akU
> > 
> > As you can see in the MemoryDumpProvider view (screenshot:
> > https://drive.google.com/open?id=0B_9WseIqRoaFR2REdkJQN20zQ1k) , there's
375.1
> +
> > 9.6 KiB from "http_cache" This number roughly matches what we see in heap
> > profiler 385.3 + 5.9 KiB (screenshot:
> > https://drive.google.com/open?id=0B_9WseIqRoaFQnRkUDU4dkZuOGM).
> 
> can you make these files World-readable please?

Ok. I moved them to chromium account and here's the public share link:
https://drive.google.com/open?id=0Bw0C8-TgzJfsakF1OVVRUy1HaWs for the three
files mentioned above.

[jkarlin, 2017-02-17 16:05:31]

jkarlin@chromium.org changed reviewers:
- gavinp@chromium.org

[jkarlin, 2017-02-17 16:05:31]

Removing gavinp@ as he is OOO for awhile. I'll take a look at net/ as well.

[jkarlin, 2017-02-17 17:43:06]

Exciting!

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:558:
base::trace_event::EstimateMemoryUsage(active_entries_);
Too bad we don't support tracing closures, as it'd be good to track
entries_pending_doom_ here.

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:558:
base::trace_event::EstimateMemoryUsage(active_entries_);
There is no EMU defined for SimpleEntryImpl. Why didn't the compiler catch that?

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:62: size_t EstimateMemoryUsage() const;
Hmm, does this get called? I don't see it called in SimpleIndex::EMU, but
perhaps it's called elsewhere?

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:114:
base::trace_event::EstimateMemoryUsage(pending_queue);
+ writer ?  EMU(writer) : 0

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:129:
base::trace_event::EstimateMemoryUsage(pending_queue);
+ disk_entry ? EMU(disk_entry) : 0

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:196: // Returns the estimate of dynamically allocated
memory in bytes.
Newline above the comment

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:508:
base::trace_event::EstimateMemoryUsage(pending_ops_);
After F2F w/ Helen we discussed that members that don't do their own allocation
are explicitly *not* being accounted for here. Instead, the idea is to only
trace objects that can grow arbitrarily because net/ just has too much to trace
otherwise.


That's why things like clock_, backend_factory_, etc. aren't accounted for.

[jkarlin, 2017-02-17 18:03:43]

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:129:
base::trace_event::EstimateMemoryUsage(pending_queue);
On 2017/02/17 17:43:06, jkarlin wrote:
> + disk_entry ? EMU(disk_entry) : 0

Actually, don't do this. We're going to count the disk_entry in the backend.

[jkarlin, 2017-02-17 18:13:08]

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:113: return
base::trace_event::EstimateMemoryUsage(readers) +
You'll need to EMU HttpCache::Transaction, which the compiler missed.

[xunjieli, 2017-02-17 22:59:09]

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

[xunjieli, 2017-02-17 22:59:11]

Dmitry: Could you take a look at the question below? Thank you.

https://codereview.chromium.org/2661333002/diff/140001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/140001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:212: DCHECK(false);
Dmitry: I followed your suggestion, but this doesn't seem to be invoked... Any
idea what might be missing?

SimpleBackendImple::EstimateMemoryUsage() does call EMU() on active_entries_
which is a map of int64 to SimpleEntryImpl*.

[DmitrySkiba, 2017-02-17 23:41:05]

On 2017/02/17 22:59:11, xunjieli wrote:
> Dmitry: Could you take a look at the question below? Thank you.
> 
>
https://codereview.chromium.org/2661333002/diff/140001/net/disk_cache/simple/...
> File net/disk_cache/simple/simple_backend_impl.cc (right):
> 
>
https://codereview.chromium.org/2661333002/diff/140001/net/disk_cache/simple/...
> net/disk_cache/simple/simple_backend_impl.cc:212: DCHECK(false);
> Dmitry: I followed your suggestion, but this doesn't seem to be invoked... Any
> idea what might be missing?
> 
> SimpleBackendImple::EstimateMemoryUsage() does call EMU() on active_entries_
> which is a map of int64 to SimpleEntryImpl*.

OK, I gave you the wrong signature. The right one is

size_t EstimateMemoryUsage(SimpleEntryImpl const* const& entry), i.e. const
reference to a const pointer.

[xunjieli, 2017-02-21 15:20:39]

Josh, thanks for the very helpful review! dskiba@ helped me with adding a static
EMU() in disk_cache namespace to track allocations from SimpleEntryImpl*. 

I uploaded a new patch. PTAL.

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:558:
base::trace_event::EstimateMemoryUsage(active_entries_);
On 2017/02/17 17:43:05, jkarlin wrote:
> Too bad we don't support tracing closures, as it'd be good to track
> entries_pending_doom_ here.

Done. I added a comment.

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2661333002/diff/100001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:62: size_t EstimateMemoryUsage() const;
On 2017/02/17 17:43:05, jkarlin wrote:
> Hmm, does this get called? I don't see it called in SimpleIndex::EMU, but
> perhaps it's called elsewhere?

Done. Good catch! I removed it

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:113: return
base::trace_event::EstimateMemoryUsage(readers) +
On 2017/02/17 18:13:08, jkarlin wrote:
> You'll need to EMU HttpCache::Transaction, which the compiler missed.

Acknowledged. We aren't tracking ActiveEntry per offline discussion

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:114:
base::trace_event::EstimateMemoryUsage(pending_queue);
On 2017/02/17 17:43:05, jkarlin wrote:
> + writer ?  EMU(writer) : 0

Acknowledged.

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:129:
base::trace_event::EstimateMemoryUsage(pending_queue);
On 2017/02/17 18:03:43, jkarlin wrote:
> On 2017/02/17 17:43:06, jkarlin wrote:
> > + disk_entry ? EMU(disk_entry) : 0
> 
> Actually, don't do this. We're going to count the disk_entry in the backend.

Acknowledged.

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:196: // Returns the estimate of dynamically allocated
memory in bytes.
On 2017/02/17 17:43:06, jkarlin wrote:
> Newline above the comment

Done.

https://codereview.chromium.org/2661333002/diff/100001/net/http/http_cache.cc...
net/http/http_cache.cc:508:
base::trace_event::EstimateMemoryUsage(pending_ops_);
On 2017/02/17 17:43:06, jkarlin wrote:
> After F2F w/ Helen we discussed that members that don't do their own
allocation
> are explicitly *not* being accounted for here. Instead, the idea is to only
> trace objects that can grow arbitrarily because net/ just has too much to
trace
> otherwise.
> 
> 
> That's why things like clock_, backend_factory_, etc. aren't accounted for.

Acknowledged. I added a comment

[xunjieli, 2017-02-21 15:20:48]

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

[commit-bot: I haz the power, 2017-02-21 15:21:09]

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

[jkarlin, 2017-02-21 16:23:29]

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.cc:144: return 0;
At least index_file_ should be counted here right?

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:62: size_t EstimateMemoryUsage() const;
Why did you remove this? Shouldn't this result in a compilation failure since
index is being tracked in httpcache?

https://codereview.chromium.org/2661333002/diff/160001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:557: (stream_0_data_ ?
stream_0_data_->capacity() : 0);
We're missing synchronous_entry_. That's difficult to capture because it lives
on multiple threads, but it could be large.

Perhaps you could include just the sizeof(synchronous_entry_) and comment that
it would be good to have the rest?

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache.cc...
net/http/http_cache.cc:504: // |backend_factory_| because they don't allocate.
Ah, not quite. We do want to track active_entries_ and doomed_entries here. We
just don't want to track ActiveEntry::disk_entry. The disk entry is tracked by
the backend.

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:8367: EXPECT_TRUE(did_dump);
Can you add a simple check that the dump size is greater than 0?

[commit-bot: I haz the power, 2017-02-21 16:58:50]

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

[commit-bot: I haz the power, 2017-02-21 16:58:51]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-02-21 19:44:14]

Thanks! PTAL

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2661333002/diff/120001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index.h:62: size_t EstimateMemoryUsage() const;
On 2017/02/21 16:23:29, jkarlin wrote:
> Why did you remove this? Shouldn't this result in a compilation failure since
> index is being tracked in httpcache?

This is not SimpleIndex. This is EntryMetaData.

https://codereview.chromium.org/2661333002/diff/160001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/disk_cache/simple/...
net/disk_cache/simple/simple_entry_impl.cc:557: (stream_0_data_ ?
stream_0_data_->capacity() : 0);
On 2017/02/21 16:23:29, jkarlin wrote:
> We're missing synchronous_entry_. That's difficult to capture because it lives
> on multiple threads, but it could be large.
> 
> Perhaps you could include just the sizeof(synchronous_entry_) and comment that
> it would be good to have the rest?

Done. I used sizeof(SimpleSynchronousEntry) because sizeof(synchronous_entry_)
is the sizeof the pointer.

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache.cc...
net/http/http_cache.cc:504: // |backend_factory_| because they don't allocate.
On 2017/02/21 16:23:29, jkarlin wrote:
> Ah, not quite. We do want to track active_entries_ and doomed_entries here. We
> just don't want to track ActiveEntry::disk_entry. The disk entry is tracked by
> the backend.

Done.

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/160001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:8367: EXPECT_TRUE(did_dump);
On 2017/02/21 16:23:29, jkarlin wrote:
> Can you add a simple check that the dump size is greater than 0?

Done.

[xunjieli, 2017-02-22 14:44:44]

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

[commit-bot: I haz the power, 2017-02-22 14:45:00]

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-22 15:45:12]

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

[commit-bot: I haz the power, 2017-02-22 15:45:14]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-02-23 15:14:57]

lgtm with a few comments

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:368:
base::trace_event::EstimateMemoryUsage(cache_);
Can you verify that the trace size is > 0?

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_file_unittest.cc:21: #include
"base/trace_event/memory_usage_estimator.h"
What's this for?

https://codereview.chromium.org/2661333002/diff/180001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/http/http_cache.cc...
net/http/http_cache.cc:115:
base::trace_event::EstimateMemoryUsage(pending_queue);
Sorry, I was wrong, this should return 0. readers and pending_queue transactions
are all owned by their respective jobs (e.g., URLRequestHttpJob) and should be
accounted for there.

[xunjieli, 2017-02-23 16:40:59]

Thanks!

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/backend...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/backend...
net/disk_cache/backend_unittest.cc:368:
base::trace_event::EstimateMemoryUsage(cache_);
On 2017/02/23 15:14:57, jkarlin wrote:
> Can you verify that the trace size is > 0?

Done.

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/disk_cache/simple/...
net/disk_cache/simple/simple_index_file_unittest.cc:21: #include
"base/trace_event/memory_usage_estimator.h"
On 2017/02/23 15:14:57, jkarlin wrote:
> What's this for?

Done. Sorry about that...

https://codereview.chromium.org/2661333002/diff/180001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

https://codereview.chromium.org/2661333002/diff/180001/net/http/http_cache.cc...
net/http/http_cache.cc:115:
base::trace_event::EstimateMemoryUsage(pending_queue);
On 2017/02/23 15:14:57, jkarlin wrote:
> Sorry, I was wrong, this should return 0. readers and pending_queue
transactions
> are all owned by their respective jobs (e.g., URLRequestHttpJob) and should be
> accounted for there.

Done. Got it. I will add those there when URLRequest*Jobs are tracked in the
MemoryDumpProvider.

[xunjieli, 2017-02-23 16:41:26]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-02-23 16:41:27]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2661333002/#ps220001
(title: "address comments")

[commit-bot: I haz the power, 2017-02-23 16:41:52]

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-23 17:45:03]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487868086884360,
"parent_rev": "d7e66510bedbf95d90337071155f8b07036cb002", "commit_rev":
"a0166f483ff1baccd7e2b0a3e8f2956c9ab1b3b6"}

[commit-bot: I haz the power, 2017-02-23 17:45:46]

Description was changed from

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.
BlockFile and In-memory cache will be implemented in a
follow-up.

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

to

==========
Track SimpleCache memory usage in net/ MemoryDumpProvider

This CL includes SimpleCache in net/ MemoryDumpProvider.
BlockFile and In-memory cache will be implemented in a
follow-up.

BUG=669108

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

[commit-bot: I haz the power, 2017-02-23 17:45:47]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/a0166f483ff1baccd7e2b0a3e8f2...

[jkarlin, 2017-02-24 18:38:10]

https://codereview.chromium.org/2661333002/diff/220001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/220001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:211: size_t
EstimateMemoryUsage(const SimpleEntryImpl* const& entry_impl) {
Have you verified that this actually gets called? I don't see it defined in the
header.

I think we need more tests to verify that everything is actually being counted.

[xunjieli, 2017-02-24 18:42:18]

https://codereview.chromium.org/2661333002/diff/220001/net/disk_cache/simple/...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/2661333002/diff/220001/net/disk_cache/simple/...
net/disk_cache/simple/simple_backend_impl.cc:211: size_t
EstimateMemoryUsage(const SimpleEntryImpl* const& entry_impl) {
On 2017/02/24 18:38:09, jkarlin wrote:
> Have you verified that this actually gets called? I don't see it defined in
the
> header.
> 
> I think we need more tests to verify that everything is actually being
counted.

I verified manually that this is called. (build chrome locally and get a trace
from chrome://tracing). The unit test (backend_unittest.cc) also enters this
code, but I am not sure what's the best way to write a stricter test expectation
(rather than using LT(0, EMU()).