[HttpCache] Store some memcache info to memory dumps for debugging

[HttpCache] Store some memcache info to memory dumps for debugging

Add some memcache variables to the memory dumps (cache max_size, amount of
physical memory, and the size the backend thinks it is) so we can debug cases
where svelte devices have large memory backends.

BUG=705053
Review-Url: https://codereview.chromium.org/2779733002
Cr-Commit-Position: refs/heads/master@{#460181}
Committed: https://chromium.googlesource.com/chromium/src/+/1f2ef177650cf95c87765a0c8a8ce4a758b5ff2a

[jkarlin, 2017-03-27 17:19:07]

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

[jkarlin, 2017-03-27 17:19:27]

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

[jkarlin, 2017-03-27 17:19:28]

xunjieli@ PTAL, thanks!

[commit-bot: I haz the power, 2017-03-27 17:19:33]

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-03-27 17:34:05]

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

[commit-bot: I haz the power, 2017-03-27 17:34:06]

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...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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-03-27 17:39:06]

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

[xunjieli, 2017-03-27 17:39:07]

Thanks, Josh. I will review this asap. +ssid: FHI.

[ssid, 2017-03-27 17:41:45]

https://codereview.chromium.org/2779733002/diff/20001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2779733002/diff/20001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:292:
pmd->CreateAllocatorDump(parent_absolute_name + "/memory_backend");
I think you should whitelist these strings in
trace_event/memory_infra_background_whitelist.cc so that they are taken by the
reporting. I *think* we have unittests also to check this.

[jkarlin, 2017-03-27 17:58:55]

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

[jkarlin, 2017-03-27 17:59:28]

https://codereview.chromium.org/2779733002/diff/20001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2779733002/diff/20001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:292:
pmd->CreateAllocatorDump(parent_absolute_name + "/memory_backend");
On 2017/03/27 17:41:45, ssid wrote:
> I think you should whitelist these strings in
> trace_event/memory_infra_background_whitelist.cc so that they are taken by the
> reporting. I *think* we have unittests also to check this.

Thanks, done!

[commit-bot: I haz the power, 2017-03-27 17:59: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-03-27 18:29:07]

lgtm with one nit and one question below.

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:330:
base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
Could you run this with base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND ?

If you do that, you can pass parent_absolute_name =
"net/url_request_context_0x123/http_cache" (or
"net/url_request_context/main/0x123/http_cache" if my CL lands before yours) to
cache_->DumpMemoryStats()

The BACKGROUND mode will additionally assert that any sub dumps created will be
whitelisted.

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

https://codereview.chromium.org/2779733002/diff/50001/net/http/http_cache.cc#...
net/http/http_cache.cc:134: // function.
If a backend is in PendingOp, does that mean the backend doesn't have much
allocations because it is being created?

[commit-bot: I haz the power, 2017-03-27 18:32:08]

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

[commit-bot: I haz the power, 2017-03-27 18:32:09]

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

[ssid, 2017-03-27 18:59:42]

Sorry, i missed the discussion about system RAM.
https://crash.corp.google.com/browse?stbtiq=649b648d20000000#2 is seen for each
report and the "Product data" section will show system info.

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:327: //
base::trace_event::EstimateMemoryUsage(cache_) is added to make sure
Is this comment still valid?

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:303:
base::SysInfo::AmountOfPhysicalMemory());
Um, we already have this data from the crash server and dashboard. I can help
you get this information if you need.
Any reason why we add this here?

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:305:
base::trace_event::MemoryAllocatorDump::kUnitsBytes,
I'd make these names as current_size and system_physical_size. Reason: the
tracing is used by everyone, not juts for debugging and descriptive names are
better for users.
Unfortunately these 2 are coupled in memory-infra.

[ssid, 2017-03-27 19:08:51]

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

[ssid, 2017-03-27 19:08:51]

also +primiano for owner

[Primiano Tucci (use gerrit), 2017-03-27 23:22:59]

Base/Trace_event whitelist lgtm

[jkarlin, 2017-03-28 13:42:43]

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

[commit-bot: I haz the power, 2017-03-28 13:42:59]

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

[jkarlin, 2017-03-28 13:44:27]

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

[jkarlin, 2017-03-28 13:44:30]

Patchset #5 (id:70001) has been deleted

[commit-bot: I haz the power, 2017-03-28 13:44:36]

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-03-28 14:16:22]

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

[commit-bot: I haz the power, 2017-03-28 14:16:22]

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

[jkarlin, 2017-03-28 14:33:52]

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

[jkarlin, 2017-03-28 14:33:54]

Patchset #5 (id:90001) has been deleted

[commit-bot: I haz the power, 2017-03-28 14:34:11]

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

[jkarlin, 2017-03-28 15:09:46]

PTAL, thanks!

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:327: //
base::trace_event::EstimateMemoryUsage(cache_) is added to make sure
On 2017/03/27 18:59:41, ssid wrote:
> Is this comment still valid?

Removed.

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:330:
base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
On 2017/03/27 18:29:06, xunjieli wrote:
> Could you run this with base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND
?
> 
> If you do that, you can pass parent_absolute_name =
> "net/url_request_context_0x123/http_cache" (or
> "net/url_request_context/main/0x123/http_cache" if my CL lands before yours)
to
> cache_->DumpMemoryStats()
> 
> The BACKGROUND mode will additionally assert that any sub dumps created will
be
> whitelisted.

Done, and broke it out into separate tests.

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_backend_impl.cc (right):

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:303:
base::SysInfo::AmountOfPhysicalMemory());
On 2017/03/27 18:59:42, ssid wrote:
> Um, we already have this data from the crash server and dashboard. I can help
> you get this information if you need.
> Any reason why we add this here?

I've removed it, but are you 100% certain that the dashboard and crash server
are getting their value with the same API?

https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_backend_impl.cc:305:
base::trace_event::MemoryAllocatorDump::kUnitsBytes,
On 2017/03/27 18:59:42, ssid wrote:
> I'd make these names as current_size and system_physical_size. Reason: the
> tracing is used by everyone, not juts for debugging and descriptive names are
> better for users.
> Unfortunately these 2 are coupled in memory-infra.

Done.

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

https://codereview.chromium.org/2779733002/diff/50001/net/http/http_cache.cc#...
net/http/http_cache.cc:134: // function.
On 2017/03/27 18:29:06, xunjieli wrote:
> If a backend is in PendingOp, does that mean the backend doesn't have much
> allocations because it is being created? 

Yep.

[jkarlin, 2017-03-28 16:22:19]

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

[commit-bot: I haz the power, 2017-03-28 16:23:15]

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

[xunjieli, 2017-03-28 16:24:59]

On 2017/03/28 15:09:46, jkarlin wrote:
> PTAL, thanks!
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
> File net/disk_cache/backend_unittest.cc (right):
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
> net/disk_cache/backend_unittest.cc:327: //
> base::trace_event::EstimateMemoryUsage(cache_) is added to make sure
> On 2017/03/27 18:59:41, ssid wrote:
> > Is this comment still valid?
> 
> Removed.
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/backend_...
> net/disk_cache/backend_unittest.cc:330:
> base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
> On 2017/03/27 18:29:06, xunjieli wrote:
> > Could you run this with
base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND
> ?
> > 
> > If you do that, you can pass parent_absolute_name =
> > "net/url_request_context_0x123/http_cache" (or
> > "net/url_request_context/main/0x123/http_cache" if my CL lands before yours)
> to
> > cache_->DumpMemoryStats()
> > 
> > The BACKGROUND mode will additionally assert that any sub dumps created will
> be
> > whitelisted.
> 
> Done, and broke it out into separate tests.
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
> File net/disk_cache/memory/mem_backend_impl.cc (right):
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
> net/disk_cache/memory/mem_backend_impl.cc:303:
> base::SysInfo::AmountOfPhysicalMemory());
> On 2017/03/27 18:59:42, ssid wrote:
> > Um, we already have this data from the crash server and dashboard. I can
help
> > you get this information if you need.
> > Any reason why we add this here?
> 
> I've removed it, but are you 100% certain that the dashboard and crash server
> are getting their value with the same API?
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/disk_cache/memory/m...
> net/disk_cache/memory/mem_backend_impl.cc:305:
> base::trace_event::MemoryAllocatorDump::kUnitsBytes,
> On 2017/03/27 18:59:42, ssid wrote:
> > I'd make these names as current_size and system_physical_size. Reason: the
> > tracing is used by everyone, not juts for debugging and descriptive names
are
> > better for users.
> > Unfortunately these 2 are coupled in memory-infra.
> 
> Done.
> 
> https://codereview.chromium.org/2779733002/diff/50001/net/http/http_cache.cc
> File net/http/http_cache.cc (right):
> 
>
https://codereview.chromium.org/2779733002/diff/50001/net/http/http_cache.cc#...
> net/http/http_cache.cc:134: // function.
> On 2017/03/27 18:29:06, xunjieli wrote:
> > If a backend is in PendingOp, does that mean the backend doesn't have much
> > allocations because it is being created? 
> 
> Yep.

lgtm. Thanks!

[commit-bot: I haz the power, 2017-03-28 16:26:54]

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

[commit-bot: I haz the power, 2017-03-28 16:26:55]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2017-03-28 16:42:20]

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

[commit-bot: I haz the power, 2017-03-28 16:42:56]

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

[xunjieli, 2017-03-28 16:43:56]

sorry for skipping base/trace_event/memory_infra_background_whitelist.cc in the
last pass.
The new patch LGTM

[jkarlin, 2017-03-28 16:45:37]

Primiano Tucci PTAL as the whitelist had to change after Helen's patch.

[Primiano Tucci (use gerrit), 2017-03-28 16:56:41]

On 2017/03/28 16:45:37, jkarlin wrote:
> Primiano Tucci PTAL as the whitelist had to change after Helen's patch.

memory_infra_background_whitelist.cc still lgtm

[jkarlin, 2017-03-28 16:58:09]

Thanks! PTAL ssid.

[ssid, 2017-03-28 17:04:16]

lgtm thanks!

[jkarlin, 2017-03-28 17:37:54]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2017-03-28 17:38:00]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2017-03-28 17:38:13]

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-03-28 19:04:43]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1490722680146240,
"parent_rev": "d9d08ba6477548fb48ce55cd1939b082d71b693c", "commit_rev":
"1f2ef177650cf95c87765a0c8a8ce4a758b5ff2a"}

[commit-bot: I haz the power, 2017-03-28 19:05:27]

Description was changed from

==========
[HttpCache] Store some memcache info to memory dumps for debugging

Add some memcache variables to the memory dumps (cache max_size, amount of
physical memory, and the size the backend thinks it is) so we can debug cases
where svelte devices have large memory backends.

BUG=705053
==========

to

==========
[HttpCache] Store some memcache info to memory dumps for debugging

Add some memcache variables to the memory dumps (cache max_size, amount of
physical memory, and the size the backend thinks it is) so we can debug cases
where svelte devices have large memory backends.

BUG=705053

Review-Url: https://codereview.chromium.org/2779733002
Cr-Commit-Position: refs/heads/master@{#460181}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f2ef177650cf95c87765a0c8a8c...
==========

[commit-bot: I haz the power, 2017-03-28 19:05:28]

Committed patchset #7 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/1f2ef177650cf95c87765a0c8a8c...