[Sync] Implement MemoryDumpProvider.

[Sync] Implement MemoryDumpProvider.

This CL adds MemoryDumpProvider for sync component to show its memory usage
in chrome://tracing.

BUG=649065
Committed: https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6
Cr-Commit-Position: refs/heads/master@{#433623}

[DmitrySkiba, 2016-10-25 21:11:08]

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

[DmitrySkiba, 2016-10-25 21:11:08]

Almost done, please take a look.

Specifically:

1. Whether any fields added to proto_visitors.h need to be excluded from 
ToValue visitor. I had to add those fields because otherwise size estimation
doesn't match real memory usage.

2. Am I acquiring the correct lock in Directory::OnMemoryDump() to access
kernel_ variable?

[pavely, 2016-10-28 22:55:33]

https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
File components/sync/protocol/proto_memory_estimations_unittest.cc (right):

https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
components/sync/protocol/proto_memory_estimations_unittest.cc:7: #include
"base/test/scoped_memory_usage.h"
I can't find scoped_memory_usage.h either.

https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
File components/sync/protocol/proto_visitors.h (right):

https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
components/sync/protocol/proto_visitors.h:554:
VISIT(client_only_encrypted_data);
We shouldn't convert client_only_encrypted_data to value as it will be added in
about:sync node browser.

https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
File components/sync/syncable/directory.cc (right):

https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
components/sync/syncable/directory.cc:19: #include
"base/trace_event/estimate_memory_usage.h"
Is estimate_memory_usage.h a new file? I can't find it.

https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
components/sync/syncable/directory.cc:914:
EstimateMemoryUsage(kernel_->metahandles_map) +
I can't see implementation of EstimateMemoryUsage, but I assume it will iterate
over collection and call EstimateMemoryUsage on values. If that's the case then
you'll need to take both locks:
  syncable::ReadTransaction trans(FROM_HERE, this);
  ScopedKernelLock lock(this);

[DmitrySkiba, 2016-10-28 22:58:52]

On 2016/10/28 22:55:33, pavely wrote:
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
> File components/sync/protocol/proto_memory_estimations_unittest.cc (right):
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
> components/sync/protocol/proto_memory_estimations_unittest.cc:7: #include
> "base/test/scoped_memory_usage.h"
> I can't find scoped_memory_usage.h either.
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
> File components/sync/protocol/proto_visitors.h (right):
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/protocol/pr...
> components/sync/protocol/proto_visitors.h:554:
> VISIT(client_only_encrypted_data);
> We shouldn't convert client_only_encrypted_data to value as it will be added
in
> about:sync node browser.
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
> File components/sync/syncable/directory.cc (right):
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
> components/sync/syncable/directory.cc:19: #include
> "base/trace_event/estimate_memory_usage.h"
> Is estimate_memory_usage.h a new file? I can't find it.
> 
>
https://codereview.chromium.org/2452713003/diff/1/components/sync/syncable/di...
> components/sync/syncable/directory.cc:914:
> EstimateMemoryUsage(kernel_->metahandles_map) +
> I can't see implementation of EstimateMemoryUsage, but I assume it will
iterate
> over collection and call EstimateMemoryUsage on values. If that's the case
then
> you'll need to take both locks:
>   syncable::ReadTransaction trans(FROM_HERE, this);
>   ScopedKernelLock lock(this);

Sorry forgot to mention, EstimateMemoryUsage CL is here:
https://codereview.chromium.org/2440393002

Will fix other things, thanks!

[DmitrySkiba, 2016-11-09 18:56:34]

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

[DmitrySkiba, 2016-11-09 18:56:35]

shess@, please take a look at sql/ changes. They add ReportMemoryUsage() to
sql::Connection, so that its users can include sqlite usage for their databases
into their memory dump. The idea is that caller creates MemoryAllocatorDump with
whatever name it wants, and calls sql::Connection::ReportMemoryUsage() to
populate that.  ReportMemoryUsage() also attributes MemoryAllocatorDump as being
sqlite-related, so that memory is not double counted.

[DmitrySkiba, 2016-11-09 19:01:48]

pavely@ please take another look. I've added code to remove
client_only_encrypted_data from PasswordSpecifics (with a unittest), and fixed
locks.

[Scott Hess - ex-Googler, 2016-11-09 20:08:33]

LGTM for sql/

I recently landed changes to enable mmap for sync ( see http://crbug.com/652359
), to fix http://crbug.com/332604 .  I would encourage reviewing the memory
usage with that in place, because it may mean that SQLite's piece is much
reduced and constant in size.  [Or at least that _should_ be the effect.]

On 2016/11/09 18:56:35, DmitrySkiba wrote:
> shess@, please take a look at sql/ changes. They add ReportMemoryUsage() to
> sql::Connection, so that its users can include sqlite usage for their
databases
> into their memory dump. The idea is that caller creates MemoryAllocatorDump
with
> whatever name it wants, and calls sql::Connection::ReportMemoryUsage() to
> populate that.  ReportMemoryUsage() also attributes MemoryAllocatorDump as
being
> sqlite-related, so that memory is not double counted.

[DmitrySkiba, 2016-11-09 20:36:47]

On 2016/11/09 20:08:33, Scott Hess wrote:
> LGTM for sql/
> 
> I recently landed changes to enable mmap for sync ( see
http://crbug.com/652359
> ), to fix http://crbug.com/332604 .  I would encourage reviewing the memory
> usage with that in place, because it may mean that SQLite's piece is much
> reduced and constant in size.  [Or at least that _should_ be the effect.]

Yes, in that bug I checked sync database specifically, and it dropped from ~1
MiB to ~30 KiB (as reported by sqlite). Still, for completeness sake I want to
include it in the sync dump.

[pavely, 2016-11-09 23:56:08]

lgtm

[DmitrySkiba, 2016-11-19 00:29:58]

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

[DmitrySkiba, 2016-11-19 00:30:19]

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

[DmitrySkiba, 2016-11-19 00:30:20]

primiano@chromium.org: Please review changes in
base/trace_event/memory_usage_estimator.h

[commit-bot: I haz the power, 2016-11-19 00:30:28]

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, 2016-11-19 00:46:33]

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

[commit-bot: I haz the power, 2016-11-19 00:46:34]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Primiano Tucci (use gerrit), 2016-11-19 01:21:52]

base/trace_event/memory_usage_estimator.h LGTM

[DmitrySkiba, 2016-11-19 06:03:58]

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

[commit-bot: I haz the power, 2016-11-19 06:04:05]

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, 2016-11-19 06:11:55]

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

[commit-bot: I haz the power, 2016-11-19 06:11:57]

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

[DmitrySkiba, 2016-11-21 18:54:37]

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

[commit-bot: I haz the power, 2016-11-21 18:55:08]

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, 2016-11-21 20:05:18]

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

[commit-bot: I haz the power, 2016-11-21 20:05:19]

Dry run: This issue passed the CQ dry run.

[DmitrySkiba, 2016-11-21 20:08:25]

The CQ bit was checked by dskiba@chromium.org

[DmitrySkiba, 2016-11-21 20:08:26]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
shess@chromium.org, primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2452713003/#ps140001
(title: "Fix presumit; fix Windows; git cl format")

[commit-bot: I haz the power, 2016-11-21 20:09:20]

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, 2016-11-21 20:16:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1479758905734510,
"parent_rev": "967f050b3855c36308cd783e5c4b030378400205", "commit_rev":
"1e474c679dcff3cc1aaf9ff974b5ff5451d23aa7"}

[commit-bot: I haz the power, 2016-11-21 20:16:57]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-21 20:18:56]

Description was changed from

==========
[Sync] Implement MemoryDumpProvider.

This CL adds MemoryDumpProvider for sync component to show its memory usage
in chrome://tracing.

BUG=649065
==========

to

==========
[Sync] Implement MemoryDumpProvider.

This CL adds MemoryDumpProvider for sync component to show its memory usage
in chrome://tracing.

BUG=649065

Committed: https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6
Cr-Commit-Position: refs/heads/master@{#433623}
==========

[commit-bot: I haz the power, 2016-11-21 20:18:57]

Patchset 8 (id:??) landed as
https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6
Cr-Commit-Position: refs/heads/master@{#433623}