[Sync] Record sync memory usage in histogram broken by datatypes

[Sync] Record sync memory usage in histogram broken by datatypes

Memory estimate is collected from Directory for directory based datatypes and
from SharedModelTypeProcessor for USS datatypes. It is triggered right after
configuration when all sync data is loaded into memory.

BUG=706214
R=pnoland@chromium.org

Review-Url: https://codereview.chromium.org/2950583003
Cr-Commit-Position: refs/heads/master@{#480992}
Committed: https://chromium.googlesource.com/chromium/src/+/0af09891c51edf00a52822907147c23b4f92aba1

[pavely, 2017-06-19 23:18:32]

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

[commit-bot: I haz the power, 2017-06-19 23:19:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Patrick Noland, 2017-06-19 23:34:52]

lgtm

https://codereview.chromium.org/2950583003/diff/20001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.cc (right):

https://codereview.chromium.org/2950583003/diff/20001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.cc:91:
SyncRecordMemoryKbHistogram("Sync.ModelTypeMemoryKB.", type(),
Can you make "Sync.ModelTypeMemoryKB." a constant? Or maybe make a helper
function that also does the division by 1024?

https://codereview.chromium.org/2950583003/diff/20001/components/sync/syncabl...
File components/sync/syncable/directory.cc (right):

https://codereview.chromium.org/2950583003/diff/20001/components/sync/syncabl...
components/sync/syncable/directory.cc:990: // Iterates over entries of |map|,
summs memory usage estimate of entries whose
[nit] sums

[pavely, 2017-06-19 23:53:38]

https://codereview.chromium.org/2950583003/diff/20001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.cc (right):

https://codereview.chromium.org/2950583003/diff/20001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.cc:91:
SyncRecordMemoryKbHistogram("Sync.ModelTypeMemoryKB.", type(),
On 2017/06/19 23:34:52, Patrick Noland wrote:
> Can you make "Sync.ModelTypeMemoryKB." a constant? Or maybe make a helper
> function that also does the division by 1024?

Done.

https://codereview.chromium.org/2950583003/diff/20001/components/sync/syncabl...
File components/sync/syncable/directory.cc (right):

https://codereview.chromium.org/2950583003/diff/20001/components/sync/syncabl...
components/sync/syncable/directory.cc:990: // Iterates over entries of |map|,
summs memory usage estimate of entries whose
On 2017/06/19 23:34:52, Patrick Noland wrote:
> [nit] sums

Done.

[pavely, 2017-06-19 23:53:43]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-06-19 23:53:43]

The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org
Link to the patchset: https://codereview.chromium.org/2950583003/#ps40001
(title: "Addressing Patrick's comments.")

[commit-bot: I haz the power, 2017-06-19 23:54: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, 2017-06-20 00:06:31]

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

[commit-bot: I haz the power, 2017-06-20 00:06:32]

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

[pavely, 2017-06-20 00:29:31]

Description was changed from

==========
[Sync] Record sync memory usage in histogram broken by datatypes

Memory estimate is collected from Directory for directory based datatypes and
from SharedModelTypeProcessor for USS datatypes. It is triggered right after
configuration when all sync data is loaded into memory.

BUG=706214
R=pnoland@chromium.org
==========

to

==========
[Sync] Record sync memory usage in histogram broken by datatypes

Memory estimate is collected from Directory for directory based datatypes and
from SharedModelTypeProcessor for USS datatypes. It is triggered right after
configuration when all sync data is loaded into memory.

BUG=706214
R=pnoland@chromium.org
==========

[pavely, 2017-06-20 00:29:31]

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

[pavely, 2017-06-20 00:30:20]

+isherman@ for tools/metrics/histograms/histograms.xml

[Ilya Sherman, 2017-06-20 20:03:36]

https://codereview.chromium.org/2950583003/diff/40001/components/sync/base/da...
File components/sync/base/data_type_histogram.cc (right):

https://codereview.chromium.org/2950583003/diff/40001/components/sync/base/da...
components/sync/base/data_type_histogram.cc:26:
base::HistogramBase::kUmaTargetedHistogramFlag);
nit: Could you please use one of the convenience wrappers from
base/metrics/histogram_functions.h?

https://codereview.chromium.org/2950583003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2950583003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75565: +<histogram
name="Sync.ModelTypeMemoryKB" units="KB">
Did you mean to list this histogram in some histogram_suffixes section?

[pavely, 2017-06-20 20:50:03]

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

[commit-bot: I haz the power, 2017-06-20 20:50:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pavely, 2017-06-20 20:51:40]

https://codereview.chromium.org/2950583003/diff/40001/components/sync/base/da...
File components/sync/base/data_type_histogram.cc (right):

https://codereview.chromium.org/2950583003/diff/40001/components/sync/base/da...
components/sync/base/data_type_histogram.cc:26:
base::HistogramBase::kUmaTargetedHistogramFlag);
On 2017/06/20 20:03:35, Ilya Sherman wrote:
> nit: Could you please use one of the convenience wrappers from
> base/metrics/histogram_functions.h?

I wanted to use UmaHistogramMemoryKB, but turned out it has a min sample of
1000. I'm going to use UmaHistogramCounts1M instead.

https://codereview.chromium.org/2950583003/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2950583003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:75565: +<histogram
name="Sync.ModelTypeMemoryKB" units="KB">
On 2017/06/20 20:03:35, Ilya Sherman wrote:
> Did you mean to list this histogram in some histogram_suffixes section?

Right, forgot about it. Thanks.

Done.

[Ilya Sherman, 2017-06-20 20:55:43]

Metrics LGTM

https://codereview.chromium.org/2950583003/diff/60001/components/sync/base/da...
File components/sync/base/data_type_histogram.cc (right):

https://codereview.chromium.org/2950583003/diff/60001/components/sync/base/da...
components/sync/base/data_type_histogram.cc:24:
base::UmaHistogramCounts1M(full_histogram_name, value / 1024);
You can also use UmaHistogramCustomCounts if you want to preserve the bounds
that you had in the previous patch set.

[pavely, 2017-06-20 21:04:22]

The CQ bit was unchecked by pavely@chromium.org

[pavely, 2017-06-20 21:04:23]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-06-20 21:04:24]

The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org
Link to the patchset: https://codereview.chromium.org/2950583003/#ps60001
(title: "Address Ilya's comments")

[commit-bot: I haz the power, 2017-06-20 21:04:44]

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-06-20 22:20:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497992663622880,
"parent_rev": "0c55ee38040de7ca13ccf95fb1c29a0227416974", "commit_rev":
"0af09891c51edf00a52822907147c23b4f92aba1"}

[commit-bot: I haz the power, 2017-06-20 22:20:21]

Description was changed from

==========
[Sync] Record sync memory usage in histogram broken by datatypes

Memory estimate is collected from Directory for directory based datatypes and
from SharedModelTypeProcessor for USS datatypes. It is triggered right after
configuration when all sync data is loaded into memory.

BUG=706214
R=pnoland@chromium.org
==========

to

==========
[Sync] Record sync memory usage in histogram broken by datatypes

Memory estimate is collected from Directory for directory based datatypes and
from SharedModelTypeProcessor for USS datatypes. It is triggered right after
configuration when all sync data is loaded into memory.

BUG=706214
R=pnoland@chromium.org

Review-Url: https://codereview.chromium.org/2950583003
Cr-Commit-Position: refs/heads/master@{#480992}
Committed:
https://chromium.googlesource.com/chromium/src/+/0af09891c51edf00a52822907147...
==========

[commit-bot: I haz the power, 2017-06-20 22:20:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0af09891c51edf00a52822907147...

[dcheng, 2017-06-21 05:25:59]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2017-06-21 05:25:59]

This seems to be causing compile errors on
https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20x86-ge....