[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and ModelTypeWorker

[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and ModelTypeWorker

This CL implements EstimateMemoryUsage for processor, worker and all subobjects.

R=pnoland@chromium.org
BUG=706214
Review-Url: https://codereview.chromium.org/2781863004
Cr-Commit-Position: refs/heads/master@{#460677}
Committed: https://chromium.googlesource.com/chromium/src/+/49e09c1721596fe0925bfd290750fd6c707b09cc

[pavely, 2017-03-29 00:24:46]

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

[commit-bot: I haz the power, 2017-03-29 00:25:23]

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-29 00:55:27]

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

[commit-bot: I haz the power, 2017-03-29 00:55:28]

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

[pavely, 2017-03-29 17:24:04]

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

[commit-bot: I haz the power, 2017-03-29 17:24: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-29 18:34:37]

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

[commit-bot: I haz the power, 2017-03-29 18:34:37]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-03-29 18:36:33]

Description was changed from

==========
[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and
ModelTypeWorker

This CL implements EstimateMemoryUsage for processor, worker and all subobjects.

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

to

==========
[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and
ModelTypeWorker

This CL implements EstimateMemoryUsage for processor, worker and all subobjects.

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

[Patrick Noland, 2017-03-29 19:42:54]

lgtm % a couple comments

https://codereview.chromium.org/2781863004/diff/20001/components/sync/base/pr...
File components/sync/base/proto_value_ptr.h (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/base/pr...
components/sync/base/proto_value_ptr.h:131: using
base::trace_event::EstimateMemoryUsage;
Can you add a comment explaining what this is for? IIRC it's to resolve the
ambiguity between two definitions of EstimateMemoryUsage?

https://codereview.chromium.org/2781863004/diff/20001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:337: size_t memory_usage = 0;
Should cryptographer be included, since the worker has a private copy?

https://codereview.chromium.org/2781863004/diff/20001/components/sync/protoco...
File components/sync/protocol/proto_visitors.h (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/protoco...
components/sync/protocol/proto_visitors.h:863: VISIT_PROTO_FIELDS(const
sync_pb::ModelTypeState& proto) {
[nit] make this alphabetic order, like you do proto_memory_estimations

[DmitrySkiba, 2017-03-29 20:01:09]

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

[DmitrySkiba, 2017-03-29 20:01:09]

Looks good, the only thing is that you shouldn't include static size (i.e.
sizeof(bla)). EstimateMemoryUsage() should return only dynamic size (i.e. what
was allocated), and static size is accounted by the outer container.

For example struct Foo { int data[100]; } doesn't allocate, so it's
EstimateMemoryUsage() should return 0. However, if you dynamically allocate it,
and call EstimateMemoryUsage(unique_ptr<Foo>) you'l get sizeof(Foo) (plus
EstimateMemoryUsage(Foo), which is 0), because that's what was allocated.

[pavely, 2017-03-30 01:07:08]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-03-30 01:07:08]

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/2781863004/#ps40001
(title: "Address comments")

[pavely, 2017-03-30 01:07:25]

https://codereview.chromium.org/2781863004/diff/20001/components/sync/base/pr...
File components/sync/base/proto_value_ptr.h (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/base/pr...
components/sync/base/proto_value_ptr.h:131: using
base::trace_event::EstimateMemoryUsage;
On 2017/03/29 19:42:54, Patrick Noland wrote:
> Can you add a comment explaining what this is for? IIRC it's to resolve the
> ambiguity between two definitions of EstimateMemoryUsage?

Done.

https://codereview.chromium.org/2781863004/diff/20001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:337: size_t memory_usage = 0;
On 2017/03/29 19:42:54, Patrick Noland wrote:
> Should cryptographer be included, since the worker has a private copy?

I Included fields that can grow arbitrarily large with number of entities. I
could add cryptographer, it would show fixed cost per datatype and would pollute
cryptographer's class with memory estimation code.

https://codereview.chromium.org/2781863004/diff/20001/components/sync/protoco...
File components/sync/protocol/proto_visitors.h (right):

https://codereview.chromium.org/2781863004/diff/20001/components/sync/protoco...
components/sync/protocol/proto_visitors.h:863: VISIT_PROTO_FIELDS(const
sync_pb::ModelTypeState& proto) {
On 2017/03/29 19:42:54, Patrick Noland wrote:
> [nit] make this alphabetic order, like you do proto_memory_estimations

This whole file is not alphabetical. Sorting it would introduce many
modifications unrelated to current CL. I'll leave this change as it is and sort
this file in a separate CL.

[commit-bot: I haz the power, 2017-03-30 01:08:15]

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-30 01:08:16]

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

[commit-bot: I haz the power, 2017-03-30 01:08:17]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[DmitrySkiba, 2017-03-30 01:10:00]

lgtm

[pavely, 2017-03-30 01:14:19]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-03-30 01:15:22]

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-30 04:53:24]

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

[commit-bot: I haz the power, 2017-03-30 04:53:25]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pavely, 2017-03-30 05:54:37]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-03-30 05:54:50]

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-30 06:08:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490853277624460,
"parent_rev": "918293741a54580488930933602b2d4984378b97", "commit_rev":
"49e09c1721596fe0925bfd290750fd6c707b09cc"}

[commit-bot: I haz the power, 2017-03-30 06:10:47]

Description was changed from

==========
[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and
ModelTypeWorker

This CL implements EstimateMemoryUsage for processor, worker and all subobjects.

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

to

==========
[Sync] Implement EstimateMemoryUsage for SharedModelTypeProcessor and
ModelTypeWorker

This CL implements EstimateMemoryUsage for processor, worker and all subobjects.

R=pnoland@chromium.org
BUG=706214

Review-Url: https://codereview.chromium.org/2781863004
Cr-Commit-Position: refs/heads/master@{#460677}
Committed:
https://chromium.googlesource.com/chromium/src/+/49e09c1721596fe0925bfd290750...
==========

[commit-bot: I haz the power, 2017-03-30 06:10:48]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/49e09c1721596fe0925bfd290750...