> 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

On 2017/02/01 01:15:52, xunjieli wrote: > > My main question here is do we have ...

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

On 2017/02/01 01:15:52, xunjieli wrote: > > My main question here is do we have ...

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

Description was changed from ========== Track SimpleCache memory usage in net/ MemoryDumpProvider This CL includes ...

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

My method is simply open a couple of tabs (with a reused profile) wait for ...

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
==========

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?

Dmitry: Could you take a look at the question below? Thank you. https://codereview.chromium.org/2661333002/diff/140001/net/disk_cache/simple/simple_backend_impl.cc File net/disk_cache/simple/simple_backend_impl.cc ...

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

Josh, thanks for the very helpful review! dskiba@ helped me with adding a static EMU() ...

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...
==========

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()).