Tag some of Mojo heap allocations for the heap profiler.

Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
TBR=sergeyu

Review-Url: https://codereview.chromium.org/2623263005
Cr-Commit-Position: refs/heads/master@{#446435}
Committed: https://chromium.googlesource.com/chromium/src/+/2207af1a2e5f829ea98198d87802872781df60b8

[Jay Civelli, 2017-01-12 20:13:27]

Description was changed from

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
==========

to

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
==========

[Jay Civelli, 2017-01-12 20:13:27]

jcivelli@chromium.org changed reviewers:
+ rockot@chromium.org, ssid@chromium.org

[Jay Civelli, 2017-01-12 20:14:58]

jcivelli@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Jay Civelli, 2017-01-12 20:33:27]

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

[commit-bot: I haz the power, 2017-01-12 20:34: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-01-12 20:44:23]

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

[commit-bot: I haz the power, 2017-01-12 20:44:25]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Ken Rockot(use gerrit already), 2017-01-12 21:05:22]

lgtm

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/connector.h (right):

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:211: // The tag to be used to track heap
allocations performed by the watcher.
nit: Not really allocations performed by the watcher

[Ken Rockot(use gerrit already), 2017-01-12 21:05:36]

(don't forget to update the watcher unittests!)

[ssid, 2017-01-12 21:50:05]

Thanks for working on this. lgtm % 1 comment.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.cc (right):

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.cc:20: heap_profiler_tag_(from_here.ToString()),
Can you please just use file_name() here instead of ToString. We currently do
not tag the allocations with each functions in the type column. This is to limit
the number of categories we have.

[ssid, 2017-01-12 21:52:54]

Missed this issue in last comment.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/connector.h (right):

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:153: void SetWatcherHeapProfilerTag(const
std::string& tag);
const char* with a comment saying this must be a const string literal and not
dynamically allocated.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:212: std::string heap_profiler_tag_;
Oh this should be const char*.
We do not store the strings in heap profiler so that we don't allocate more
memory for storing category names. It is better if we are tagged with only
strings in the binary. So, const char* everywhere.

[Jay Civelli, 2017-01-13 01:09:28]

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/connector.h (right):

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:153: void SetWatcherHeapProfilerTag(const
std::string& tag);
On 2017/01/12 21:52:54, ssid wrote:
> const char* with a comment saying this must be a const string literal and not
> dynamically allocated.

Done.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:211: // The tag to be used to track heap
allocations performed by the watcher.
On 2017/01/12 21:05:22, Ken Rockot wrote:
> nit: Not really allocations performed by the watcher

Clarified.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/connector.h:212: std::string heap_profiler_tag_;
On 2017/01/12 21:52:54, ssid wrote:
> Oh this should be const char*.
> We do not store the strings in heap profiler so that we don't allocate more
> memory for storing category names. It is better if we are tagged with only
> strings in the binary. So, const char* everywhere.

Done.

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.cc (right):

https://codereview.chromium.org/2623263005/diff/20001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.cc:20: heap_profiler_tag_(from_here.ToString()),
On 2017/01/12 21:50:03, ssid wrote:
> Can you please just use file_name() here instead of ToString. We currently do
> not tag the allocations with each functions in the type column. This is to
limit
> the number of categories we have.

Done.

https://codereview.chromium.org/2623263005/diff/40001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.cc (right):

https://codereview.chromium.org/2623263005/diff/40001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.cc:86:
TRACE_HEAP_PROFILER_API_SCOPED_TASK_EXECUTION event(heap_profiler_tag_);
I moved the macro here so it catches allocation when posting.
Is that the right thing to do? @ssid

[ssid, 2017-01-13 01:46:17]

https://codereview.chromium.org/2623263005/diff/40001/mojo/public/cpp/system/...
File mojo/public/cpp/system/watcher.cc (right):

https://codereview.chromium.org/2623263005/diff/40001/mojo/public/cpp/system/...
mojo/public/cpp/system/watcher.cc:86:
TRACE_HEAP_PROFILER_API_SCOPED_TASK_EXECUTION event(heap_profiler_tag_);
On 2017/01/13 01:09:28, Jay Civelli wrote:
> I moved the macro here so it catches allocation when posting.
> Is that the right thing to do? @ssid

I just noticed the post task.
Yes this would capture more allocations, than CallOnHandleReady.
Is this the only call site where the handler can be called?

[Ken Rockot(use gerrit already), 2017-01-13 01:53:34]

On Jan 12, 2017 5:46 PM, <ssid@chromium.org> wrote:


https://codereview.chromium.org/2623263005/diff/40001/
mojo/public/cpp/system/watcher.cc
File mojo/public/cpp/system/watcher.cc (right):

https://codereview.chromium.org/2623263005/diff/40001/
mojo/public/cpp/system/watcher.cc#newcode86
mojo/public/cpp/system/watcher.cc:86:
TRACE_HEAP_PROFILER_API_SCOPED_TASK_EXECUTION event(heap_profiler_tag_);
On 2017/01/13 01:09:28, Jay Civelli wrote:
> I moved the macro here so it catches allocation when posting.
> Is that the right thing to do? @ssid

I just noticed the post task.
Yes this would capture more allocations, than CallOnHandleReady.
Is this the only call site where the handler can be called?


Drive-by owner comment: yes, it is.


https://codereview.chromium.org/2623263005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jay Civelli, 2017-01-13 02:05:14]

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

[commit-bot: I haz the power, 2017-01-13 02:05:42]

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-01-13 02:15:13]

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

[commit-bot: I haz the power, 2017-01-13 02:15:14]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Jay Civelli, 2017-01-15 18:23:48]

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

[commit-bot: I haz the power, 2017-01-15 18:23:59]

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-01-15 18:33:17]

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

[commit-bot: I haz the power, 2017-01-15 18:33:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Jay Civelli, 2017-01-15 20:10:05]

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

[commit-bot: I haz the power, 2017-01-15 20:10:12]

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-01-15 20:17:22]

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

[commit-bot: I haz the power, 2017-01-15 20:17:23]

Dry run: 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...)

[Jay Civelli, 2017-01-15 23:59:03]

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

[commit-bot: I haz the power, 2017-01-15 23:59:11]

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-01-16 01:41:15]

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

[commit-bot: I haz the power, 2017-01-16 01:41:16]

Dry run: This issue passed the CQ dry run.

[Jay Civelli, 2017-01-16 01:45:07]

Patchset #5 (id:80001) has been deleted

[Jay Civelli, 2017-01-17 17:55:29]

jcivelli@chromium.org changed reviewers:
+ creis@chromium.org, rohitrao@chromium.org, sandersd@chromium.org,
tommi@chromium.org

[Jay Civelli, 2017-01-17 17:55:29]

Adding owner reviewers:
sandersd@ could you look at media/?
rohitrao@ could you look at ios/?
creis@ could you look at content/?
tommi@ could you look at chrome/browser/media/?

[Charlie Reis, 2017-01-17 19:35:21]

RS LGTM for content/.

[sandersd (OOO until July 31), 2017-01-17 22:02:25]

media/ lgtm.

[rohitrao (ping after 24h), 2017-01-18 15:25:50]

ios lgtm

[Jay Civelli, 2017-01-18 20:21:33]

jcivelli@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Jay Civelli, 2017-01-18 20:22:23]

Adding sergeyu@ from OWNER review of chrome/browser/media

[Jay Civelli, 2017-01-25 22:47:36]

Description was changed from

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
==========

to

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
TBR=sergeyu
==========

[Jay Civelli, 2017-01-25 22:47:43]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2017-01-25 22:47:44]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
ssid@chromium.org, sandersd@chromium.org, creis@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2623263005/#ps120001
(title: "Synced")

[commit-bot: I haz the power, 2017-01-25 22:48:48]

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-01-26 02:40:55]

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

[commit-bot: I haz the power, 2017-01-26 02:40:57]

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

[Jay Civelli, 2017-01-26 18:33:04]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2017-01-26 18:33:45]

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-01-26 20:46:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485455584569110,
"parent_rev": "99a75259cfd52d1390fc0733f9dc18326c819f6f", "commit_rev":
"2207af1a2e5f829ea98198d87802872781df60b8"}

[commit-bot: I haz the power, 2017-01-26 20:46:40]

Description was changed from

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
TBR=sergeyu
==========

to

==========
Tag some of Mojo heap allocations for the heap profiler.

First step in tagging heap allocations for Mojo as to provide context
for the heap profiler.
Allocations done by mojo::Watcher are now tagged with the master
interface name if available, otherwise with the location where the
Watcher was allocated.
Non Mojo IPCs are tagged as "IPC Channel" at this point.

BUG=676682
TBR=sergeyu

Review-Url: https://codereview.chromium.org/2623263005
Cr-Commit-Position: refs/heads/master@{#446435}
Committed:
https://chromium.googlesource.com/chromium/src/+/2207af1a2e5f829ea98198d87802...
==========

[commit-bot: I haz the power, 2017-01-26 20:46:41]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2207af1a2e5f829ea98198d87802...