Add a Mojo MemoryDumpProvider.

Add a Mojo MemoryDumpProvider.

The MemoryDumpProvider counts Dispatchers and segments them by type.

BUG=705785
Review-Url: https://codereview.chromium.org/2915013006
Cr-Commit-Position: refs/heads/master@{#476870}
Committed: https://chromium.googlesource.com/chromium/src/+/ea7ac132bcc7d34cff4b68e9d19ba0225d27270a

[erikchen, 2017-06-02 22:05:11]

erikchen@chromium.org changed reviewers:
+ rockot@chromium.org

[erikchen, 2017-06-02 22:05:12]

rockot: Please review.

[erikchen, 2017-06-02 22:06:42]

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

[commit-bot: I haz the power, 2017-06-02 22:07:32]

Dry run: CQ is trying da patch.

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

[Ken Rockot(use gerrit already), 2017-06-02 22:15:51]

I think we could probably refactor HandleTable a bit so it doesn't need to
expose its internal Lock, but I can always do that some other time. LGTM,
thanks!

[erikchen, 2017-06-02 22:19:44]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-06-02 22:19:44]

The CQ bit was checked by erikchen@chromium.org

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

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-02 22:59:18]

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

[commit-bot: I haz the power, 2017-06-02 22:59:19]

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

[erikchen, 2017-06-02 23:16:44]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-06-02 23:16:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2915013006/#ps20001
(title: "compile error.")

[commit-bot: I haz the power, 2017-06-02 23:17:36]

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-03 01:14:54]

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

[commit-bot: I haz the power, 2017-06-03 01:14:55]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[erikchen, 2017-06-03 01:26:30]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-06-03 01:26:54]

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-03 02:41:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496453190282310,
"parent_rev": "089996867fb18c8e7275fef9b4ced99e19d8acac", "commit_rev":
"ea7ac132bcc7d34cff4b68e9d19ba0225d27270a"}

[commit-bot: I haz the power, 2017-06-03 02:41:08]

Description was changed from

==========
Add a Mojo MemoryDumpProvider.

The MemoryDumpProvider counts Dispatchers and segments them by type.

BUG=705785
==========

to

==========
Add a Mojo MemoryDumpProvider.

The MemoryDumpProvider counts Dispatchers and segments them by type.

BUG=705785

Review-Url: https://codereview.chromium.org/2915013006
Cr-Commit-Position: refs/heads/master@{#476870}
Committed:
https://chromium.googlesource.com/chromium/src/+/ea7ac132bcc7d34cff4b68e9d19b...
==========

[commit-bot: I haz the power, 2017-06-03 02:41:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/ea7ac132bcc7d34cff4b68e9d19b...

[ssid, 2017-06-07 00:30:28]

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

[ssid, 2017-06-07 00:30:29]

Sorry for late review. Please add primiano / me for review when you are adding a
new dump provider.

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:35: return "unknown";
Can you add NOTREACHED() here?
Creating 2 memory dumps with same name will fail and if it fails in
ProcessMemoryDump then we will be confused why.

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:38: const char* kDumpProviderName =
"MojoHandleTable";
remove this constant here and just inline. Will be used at one place only.

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:48:
base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
Why are you registering the dump provider again. This is supposed to be
unregister?

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:164: bool HandleTable::OnMemoryDump(const
base::trace_event::MemoryDumpArgs& args,
Is it possible to add test for this?
If this is to be enabled in slow reports then it's better to have tests.

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:174:
handle_count[Dispatcher::Type::PLATFORM_HANDLE];
These lines are not needed.
Just doing ++handle_count[entry.second.dispatcher->GetType()] will initialize
first entry to 0.

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:187:
outer_dump->AddScalar(GetNameForDispatcherType(entry.first),
This should be "mojo/unknown" and so on. Else no size will be shown in the UI.

[ssid, 2017-06-07 00:31:16]

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
File mojo/edk/system/handle_table.cc (right):

https://codereview.chromium.org/2915013006/diff/20001/mojo/edk/system/handle_...
mojo/edk/system/handle_table.cc:187:
outer_dump->AddScalar(GetNameForDispatcherType(entry.first),
On 2017/06/07 00:30:28, ssid wrote:
> This should be "mojo/unknown" and so on. Else no size will be shown in the UI.

just noticed that this is fixed. please ignore.