New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 649065 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit 15 days ago
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: ----



Sign in to add a comment

Add MemoryDumpProvider for sync

Project Member Reported by dskiba@chromium.org, Sep 21 2016

Issue description

components/sync can be responsible for allocating several MiBs in Chrome's browser process, but since there is no MemoryDumpProvider for sync, its memory usage contribution can be seen only in heap profiler.

We need to implement MemoryDumpProvider for sync to make it visible in telemetry / slow reports / etc.

Here are example usage of various sync components as captured by native heap profiler on my private Chrome profile:

3.4 KiB      components/web_cache/browser
22.3 KiB     components/sync/engine_impl
2,954.0 KiB  components/sync/driver/glue
436.5 KiB    components/sync/driver
1.5 KiB      components/sync/core_impl
1.1 KiB      components/sync/core
3.7 KiB      components/sync/base

Top functions:

215.6 KiB    browser_sync::FaviconCache::MergeDataAndStartSyncing()
2,825.2 KiB  syncer::syncable::Directory::OpenImpl()

Trace file: https://drive.google.com/file/d/0B_Hmi138MnbJc2ZWUXBlMlo3Rlk/view?usp=sharing
 

Comment 1 by dskiba@chromium.org, Sep 21 2016

Description: Show this description

Comment 2 by dskiba@chromium.org, Oct 19 2016

Detailed breakdown of Directory::OpenImpl():

2,825.2 KiB syncer::syncable::Directory::OpenImpl
2,552.8 KiB   syncer::syncable::OnDiskDirectoryBackingStore::Load
2,552.8 KiB     syncer::syncable::OnDiskDirectoryBackingStore::TryLoad
2,469.7 KiB       syncer::syncable::DirectoryBackingStore::LoadEntries
1,520.9 KiB         syncer::syncable::UnpackEntry
652.4 KiB             google::protobuf::MessageLite::ParseFromArray
489.2 KiB             ShimCppNew
190.3 KiB             sql::Statement::ColumnString
165.7 KiB             std::__1::basic_string::operator=
18.1 KiB              syncer::UniquePosition::FromProto
5.2 KiB               syncer::syncable::EntryKernel::ShouldMaintainPosition
900.6 KiB           sql::Statement::Step
900.6 KiB             sql::Statement::StepInternal

I.e. EntryKernel objects (which UnpackEntry() creates) are responsible for 1,520.9 KiB, and 652.4 KiB of that comes from protobuf messages.
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05476f9a8e8c076a8752b549d14269a6eca006c1

commit 05476f9a8e8c076a8752b549d14269a6eca006c1
Author: dskiba <dskiba@google.com>
Date: Fri Oct 21 00:07:05 2016

[Sync] Reimplement proto value conversions on top of field visitors.

Sync component can allocate nontrivial amounts of memory, and browser
memory team would like to add MemoryDumpProvider to track sync memory
usage.

While ~1/4 of all used memory comes from protobuf messages (see the bug),
there is no easy way to get memory usage of a proto message. All we can
do is to estimate on per-field basis. For that we need a way to enumerate
fields of a given proto.

Fortunately, proto_value_conversions.cc already has functions that go
through fields for all sync protos. The only problem is that those
functions (ProtoToValue) are tied to a specific purpose of generating
base::DictionaryValues.

This CL refactors field enumeration out of ProtoToValue functions so that
next CL could use that to estimate proto memory usage.

In particular, this CL:

1. Extracts field enumeration from ProtoToValue functions into
   VisitProtoFields(visitor, proto) function templates.

2. Implements ToValueVisitor that uses VisitProtoFields() to convert
   protos to base::DictionaryValues. ToValueVisitor also includes all
   logic from ProtoToValue functions.

3. Reimplements ProtoToValue() functions on top of ToValueVisitor.

Additionally this CL renames functions that convert proto enums to strings
to better play with templates.

BUG= 649065 

Review-Url: https://chromiumcodereview.appspot.com/2433563002
Cr-Commit-Position: refs/heads/master@{#426655}

[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/BUILD.gn
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/driver/about_sync_util.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/engine/cycle/sync_cycle_snapshot.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/engine/events/configure_get_updates_request_event.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_enum_conversions.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_enum_conversions.h
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_enum_conversions_unittest.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_value_conversions.cc
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_value_conversions.h
[modify] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_value_conversions_unittest.cc
[add] https://crrev.com/05476f9a8e8c076a8752b549d14269a6eca006c1/components/sync/protocol/proto_visitors.h

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d17ffd41bac39d40a9f351929f1a87ace0775961

commit d17ffd41bac39d40a9f351929f1a87ace0775961
Author: dskiba <dskiba@google.com>
Date: Fri Nov 11 20:29:09 2016

[tracing] Implement composable memory usage estimators.

When writing MemoryDumpProviders it's sometimes necessary to know how
much memory a particular class is responsible for. The answer is not easy
when class' memory usage is spread across several members which use STL
containers.

This CL introduces EstimateMemoryUsage() family of methods to accurately
estimate memory usage of STL containers. I.e. if EstimateMemoryUsage() is
implemented for type T, the memory usage of std::map<int, std::vector<T>>
is just one EstimateMemoryUsage() call away (instead of two nested loops),
and also includes memory used by std::map/vector (which can be big!).

The change is needed for sync MDP provider, but can also be used in other
MDPs. As an example this CL converts "ui/resource_manager" MDP to use
estimators resulting in cleaner code.

BUG= 649065 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2440393002
Cr-Commit-Position: refs/heads/master@{#431622}

[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/base/BUILD.gn
[add] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/base/trace_event/memory_usage_estimator.cc
[add] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/base/trace_event/memory_usage_estimator.h
[add] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/base/trace_event/memory_usage_estimator_unittest.cc
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/cc/resources/scoped_ui_resource.h
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/cc/resources/ui_resource_bitmap.h
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/ui/android/resources/crushed_sprite_resource.cc
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/ui/android/resources/crushed_sprite_resource.h
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/ui/android/resources/resource_manager.cc
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/ui/android/resources/resource_manager.h
[modify] https://crrev.com/d17ffd41bac39d40a9f351929f1a87ace0775961/ui/android/resources/resource_manager_impl.cc

Comment 5 by dskiba@chromium.org, Nov 18 2016

The sync MDP is quite accurate: on my personal profile it reports 2.3 MiB, while all components/sync categories are 2.44 MiB. 

Comment 6 by zea@chromium.org, Nov 19 2016

Cc: gangwu@chromium.org
Awesome, thanks Dmitry! +Gang FYI
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 21 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4199f82689fd146d52d8352f4d07bd79d6cfbd6

commit b4199f82689fd146d52d8352f4d07bd79d6cfbd6
Author: dskiba <dskiba@chromium.org>
Date: Mon Nov 21 20:16:13 2016

[Sync] Implement MemoryDumpProvider.

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

BUG= 649065 

Review-Url: https://codereview.chromium.org/2452713003
Cr-Commit-Position: refs/heads/master@{#433623}

[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/base/trace_event/memory_usage_estimator.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/BUILD.gn
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/base/proto_value_ptr.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/base/unique_position.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/base/unique_position.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/driver/glue/sync_backend_host_core.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/engine/fake_sync_manager.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/engine/fake_sync_manager.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/engine/sync_manager.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/engine_impl/sync_manager_impl.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/engine_impl/sync_manager_impl.h
[add] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/protocol/proto_memory_estimations.cc
[add] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/protocol/proto_memory_estimations.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/protocol/proto_value_conversions.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/protocol/proto_value_conversions_unittest.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/protocol/proto_visitors.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/directory.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/directory.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/directory_backing_store.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/directory_backing_store.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/entry_kernel.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/entry_kernel.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/parent_child_index.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/parent_child_index.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/components/sync/syncable/syncable_id.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/sql/connection.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/sql/connection.h
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/sql/connection_memory_dump_provider.cc
[modify] https://crrev.com/b4199f82689fd146d52d8352f4d07bd79d6cfbd6/sql/connection_memory_dump_provider.h

Comment 8 by dskiba@chromium.org, Nov 28 2016

Almost done, just need to enable BACKGROUND mode...
Project Member

Comment 9 by bugdroid1@chromium.org, Dec 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/caffce579a5c78540e15a0a280f029abb1781818

commit caffce579a5c78540e15a0a280f029abb1781818
Author: dskiba <dskiba@chromium.org>
Date: Thu Dec 01 21:26:16 2016

[tracing] Whitelist sync MDP.

Sync MDP takes longer than other whitelisted providers (~10ms on Nexus 5X),
but that shouldn't affect usability because the provider runs on dedicated
thread (SyncThread).

BUG= 649065 

Review-Url: https://codereview.chromium.org/2537903002
Cr-Commit-Position: refs/heads/master@{#435717}

[modify] https://crrev.com/caffce579a5c78540e15a0a280f029abb1781818/base/trace_event/memory_infra_background_whitelist.cc

Status: Fixed (was: Started)

Sign in to add a comment