[tracing] Add WebCache memory usage in Blink to chrome://tracing

[tracing] Add WebCache memory usage in Blink to chrome://tracing

WebCache stores the web content downloaded from the internet in
memory. This CL adds memory statistics of WebCache to tracing.

BUG=466141
Committed: https://crrev.com/65f8b7693255ae50ac6bdbbf9679bb88dc3733cb
Cr-Commit-Position: refs/heads/master@{#343350}

[ssid, 2015-08-11 17:40:01]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2015-08-11 17:40:02]

PTAL, Thanks

https://codereview.chromium.org/1283793004/diff/40001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.cc (right):

https://codereview.chromium.org/1283793004/diff/40001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:217:
content::ChildProcessHostImpl::ChildProcessUniqueIdToTracingProcessId(
This causes a DEPS addition to component which is not really desired. But, I
couldn't find other choices! Maybe send the function as callback to initialize
this component?

https://codereview.chromium.org/1283793004/diff/40001/content/child/DEPS
File content/child/DEPS (right):

https://codereview.chromium.org/1283793004/diff/40001/content/child/DEPS#newc...
content/child/DEPS:9: "+components/web_cache/renderer",
I don't think adding here is a big issue.

[Primiano Tucci (use gerrit), 2015-08-12 10:04:45]

Hmm let's have an offline chat about this.
You are treating this as cross-process shared memory but I don't think this is
the case.
My understanding is that the browser just does policy and bookkeeping but the
actual caches (MemoryCache.cpp) is per-renderer only.

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
File components/web_cache/browser/DEPS (right):

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/DEPS:2: "+content/common",
Hmm I'm not sure you can depend on content/common from a component. I think you
can access only the public conent API surface.

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.cc (right):

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:76: this);
don't you need thread runner affinity here?

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:195: while
(active_renderers_iter != active_renderers_.end()) {
I think lines 193-195 could be just:
for (int renderer_id : active_renderers_)

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:200:
std::set<int>::const_iterator inactive_renderers_iter =
same here

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:214: if (stats ==
stats_.end())
Are you expecting this to happen genuinely for some reason?
If not just make this a DCHCEK

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.h (right):

https://codereview.chromium.org/1283793004/diff/60001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.h:227: void DumpUsageStats(int
renderer_id,
nit: how about DumpMemoryStatsForRenderer ?

[ssid, 2015-08-12 11:57:09]

Thanks. After discussion offline, made changes, PTAL.

[ssid, 2015-08-12 17:06:40]

On 2015/08/12 11:57:09, ssid wrote:
> Thanks. After discussion offline, made changes, PTAL.

Please make sure you have https://codereview.chromium.org/1278403007/ in the
checkout if you are testing.

[Primiano Tucci (use gerrit), 2015-08-13 13:30:21]

LGTM with some minor nits.
I am not a webcache expert but the code makes sense to me
Please make sure you get a review from a webcache owner and that hey are onboard
with the logic here.

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
File components/web_cache/renderer/web_cache_memory_dump_provider.cc (right):

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:15: void
DumpResourceStats(blink::WebCache::ResourceTypeStat resource_stat,
No need to pass a copy. Add const...&

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:16: const
std::string resource_name,
same here

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:73:
blink::WebCache::ResourceTypeStats resource_stats;
 maybe = {0} also here for consistency?

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:66:
webkit_initialized_ = false;
Just for consistency, I'd make the register and unregister sequence symmetric.
In other words, I'd move the webkit_initialized = false below

[ssid, 2015-08-13 15:25:34]

Thanks

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
File components/web_cache/renderer/web_cache_memory_dump_provider.cc (right):

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:15: void
DumpResourceStats(blink::WebCache::ResourceTypeStat resource_stat,
On 2015/08/13 13:30:21, Primiano Tucci wrote:
> No need to pass a copy. Add const...&

Done.

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:16: const
std::string resource_name,
On 2015/08/13 13:30:21, Primiano Tucci wrote:
> same here

Done.

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:73:
blink::WebCache::ResourceTypeStats resource_stats;
On 2015/08/13 13:30:21, Primiano Tucci wrote:
>  maybe = {0} also here for consistency?

Um, it is struct of structs, so it looks dirty. The api just replaces the data
anyways, so removing the previous init to be consistent.

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/1283793004/diff/80001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:66:
webkit_initialized_ = false;
On 2015/08/13 13:30:21, Primiano Tucci wrote:
> Just for consistency, I'd make the register and unregister sequence symmetric.
> In other words, I'd move the webkit_initialized = false below

Done.

[ssid, 2015-08-13 15:33:54]

ssid@chromium.org changed reviewers:
+ jam@chromium.org, thestig@chromium.org

[ssid, 2015-08-13 15:33:54]

This CL adds memory tracing of web cache.

+thestig for web_cache component.

+jam for components/web_cache.gypi file additions.

Thanks

[Primiano Tucci (use gerrit), 2015-08-13 17:30:06]

On 2015/08/13 15:33:54, ssid wrote:
> This CL adds memory tracing of web cache.
> 
> +thestig for web_cache component.
> 
> +jam for components/web_cache.gypi file additions.
> 
> Thanks

Ah, I just realized something by looking at the description of this patch.
COnversely to what you say, this doesn't seem to have any ownership edge. You
can do in a follow up CL, but should figure out where webcache memory is
allocated from (malloc? partalloc?) and add the relationship there.

[Lei Zhang, 2015-08-13 18:44:56]

On 2015/08/13 17:30:06, Primiano Tucci wrote:
> Ah, I just realized something by looking at the description of this patch.
> COnversely to what you say, this doesn't seem to have any ownership edge. You
> can do in a follow up CL, but should figure out where webcache memory is
> allocated from (malloc? partalloc?) and add the relationship there.

Also extra word? "WebCache is stores ..."

[Lei Zhang, 2015-08-13 18:48:42]

https://codereview.chromium.org/1283793004/diff/100001/components/web_cache/r...
File components/web_cache/renderer/web_cache_memory_dump_provider.h (right):

https://codereview.chromium.org/1283793004/diff/100001/components/web_cache/r...
components/web_cache/renderer/web_cache_memory_dump_provider.h:8: #include
"base/memory/singleton.h"
"Do you really need a singleton?" as seen in singleton.h. See also:
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

[ssid, 2015-08-13 19:52:38]

Patchset #7 (id:120001) has been deleted

[ssid, 2015-08-13 20:27:14]

Patchset #7 (id:140001) has been deleted

[ssid, 2015-08-13 21:09:12]

On 2015/08/13 18:48:42, Lei Zhang wrote:
>
https://codereview.chromium.org/1283793004/diff/100001/components/web_cache/r...
> File components/web_cache/renderer/web_cache_memory_dump_provider.h (right):
> 
>
https://codereview.chromium.org/1283793004/diff/100001/components/web_cache/r...
> components/web_cache/renderer/web_cache_memory_dump_provider.h:8: #include
> "base/memory/singleton.h"
> "Do you really need a singleton?" as seen in singleton.h. See also:
>
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

Thanks, changed to LazyInstance. PTAL.

[Lei Zhang, 2015-08-13 21:14:39]

lgtm

https://codereview.chromium.org/1283793004/diff/180001/components/web_cache/r...
File components/web_cache/renderer/web_cache_memory_dump_provider.cc (right):

https://codereview.chromium.org/1283793004/diff/180001/components/web_cache/r...
components/web_cache/renderer/web_cache_memory_dump_provider.cc:48: return
&g_wcmdp_instance.Get();
g_wcmdp_instance.Pointer() ?

[jam, 2015-08-13 21:31:33]

lgtm

[ssid, 2015-08-14 07:10:17]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-14 07:10:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, thestig@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1283793004/#ps200001
(title: "get() -> pointer()")

[commit-bot: I haz the power, 2015-08-14 07:10:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1283793004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1283793004/200001

[commit-bot: I haz the power, 2015-08-14 07:53:46]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-08-14 07:54:45]

Patchset 9 (id:??) landed as
https://crrev.com/65f8b7693255ae50ac6bdbbf9679bb88dc3733cb
Cr-Commit-Position: refs/heads/master@{#343350}