Launch browser when --memlog is specified on the command line.

Launch browser when --memlog is specified on the command line.

This communicates a pipe name to the launched process which is propagated to child processes. It is not yet used.

The current process topology is the profiling process launches the browser which then runs like normal. In a later pass I will change this to have the browser process launch the profiling process when the flag is set. I believe this topology will be required for certain platforms. But it will require more plumbing for "process hosts" and mojo pipes that will take more time and don't need to block the current work.

BUG=733336
R=ajwong@chromium.org, borisv@chromium.org

Review-Url: https://codereview.chromium.org/2937133002 .
Cr-Commit-Position: refs/heads/master@{#479754}
Committed: https://chromium.googlesource.com/chromium/src/+/1664378033de651755a3b4e46ec9690600dc8873

[brettw, 2017-06-14 23:05:20]

brettw@chromium.org changed reviewers:
+ borisv@chromium.org

[brettw, 2017-06-14 23:05:24]

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

[commit-bot: I haz the power, 2017-06-14 23:05:49]

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-06-14 23:08:09]

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

[commit-bot: I haz the power, 2017-06-14 23:08:10]

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 on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brettw, 2017-06-14 23:13:14]

Description was changed from

==========
Launch browser when --memlog is specified on the command line.

This communicates a pipe name to the launched process which is propagated to
child processes. It is not yet used.

The current process topology is the profiling process launches the browser which
then runs like normal. In a later pass I will change this to have the browser
process launch the profiling process when the flag is set. I believe this
topology will be required for certain platforms. But it will require more
plumbing for "process hosts" and mojo pipes that will take more time and don't
need to block the current work.
==========

to

==========
Launch browser when --memlog is specified on the command line.

This communicates a pipe name to the launched process which is propagated to
child processes. It is not yet used.

The current process topology is the profiling process launches the browser which
then runs like normal. In a later pass I will change this to have the browser
process launch the profiling process when the flag is set. I believe this
topology will be required for certain platforms. But it will require more
plumbing for "process hosts" and mojo pipes that will take more time and don't
need to block the current work.

BUG=733336
==========

[awong, 2017-06-14 23:15:18]

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

[awong, 2017-06-14 23:15:19]

https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1134: const char kMemlog[] = "memlog";
should these be constexprs in the header now?

legitimate question...I don't know my c++11 well enough...

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
File chrome/profiling/profiling_globals.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
chrome/profiling/profiling_globals.cc:15: :
io_thread_(base::MakeUnique<base::Thread>("IO thread")) {
Don't change this code, but do we actually want an I/O thread in the future?

Or can we just use one message loop such that the UI thread *is* an IO thread?


Also, why is this a unique_ptr<>?  Can't we just contain the base::Thread
directly?

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
File chrome/profiling/profiling_main.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
chrome/profiling/profiling_main.cc:100: if (!LaunchBrowser(cmdline, pipe_id))
Wait, so we're starting the metrics process _before_ the browser?

I thought we were have browser spawn it early in the process?

With this design, we will break gdb attachment no?

[awong, 2017-06-14 23:15:59]

LGTM enough for initial commit. We can continue to correct as we go.
On 2017/06/14 23:15:19, awong wrote:
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
> File chrome/common/chrome_switches.cc (right):
> 
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
> chrome/common/chrome_switches.cc:1134: const char kMemlog[] = "memlog";
> should these be constexprs in the header now?
> 
> legitimate question...I don't know my c++11 well enough...
> 
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
> File chrome/profiling/profiling_globals.cc (right):
> 
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
> chrome/profiling/profiling_globals.cc:15: :
> io_thread_(base::MakeUnique<base::Thread>("IO thread")) {
> Don't change this code, but do we actually want an I/O thread in the future?
> 
> Or can we just use one message loop such that the UI thread *is* an IO thread?
> 
> 
> Also, why is this a unique_ptr<>?  Can't we just contain the base::Thread
> directly?
> 
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
> File chrome/profiling/profiling_main.cc (right):
> 
>
https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
> chrome/profiling/profiling_main.cc:100: if (!LaunchBrowser(cmdline, pipe_id))
> Wait, so we're starting the metrics process _before_ the browser?
> 
> I thought we were have browser spawn it early in the process?
> 
> With this design, we will break gdb attachment no?

[brettw, 2017-06-14 23:23:55]

https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/common/chrome_sw...
chrome/common/chrome_switches.cc:1134: const char kMemlog[] = "memlog";
I don't think so, since that will inline them.

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
File chrome/profiling/profiling_globals.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
chrome/profiling/profiling_globals.cc:15: :
io_thread_(base::MakeUnique<base::Thread>("IO thread")) {
On 2017/06/14 23:15:19, awong wrote:
> Don't change this code, but do we actually want an I/O thread in the future?
> 
> Or can we just use one message loop such that the UI thread *is* an IO thread?
> 
> 
> Also, why is this a unique_ptr<>?  Can't we just contain the base::Thread
> directly?

I changed it. I did it that way because I was thinking we would want to control
the lifetime of the thread more precisely.

I added a note about changing the main thread to be the I/O thread in the
future. Not sure what we'll do with the main thread yet.

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
File chrome/profiling/profiling_main.cc (right):

https://codereview.chromium.org/2937133002/diff/20001/chrome/profiling/profil...
chrome/profiling/profiling_main.cc:100: if (!LaunchBrowser(cmdline, pipe_id))
On 2017/06/14 23:15:19, awong wrote:
> Wait, so we're starting the metrics process _before_ the browser?
> 
> I thought we were have browser spawn it early in the process?
> 
> With this design, we will break gdb attachment no?

Yes. It makes sense if you think of running chrome in an "instrumented
environment" (what I was thinking when I wrote this). But I agree it's not what
we want.

[Boris Vidolov, 2017-06-14 23:56:15]

lgtm

lgtm

[brettw, 2017-06-14 23:56:31]

Fix

[brettw, 2017-06-15 03:50:36]

The CQ bit was checked by brettw@chromium.org

[brettw, 2017-06-15 03:50:37]

The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org,
borisv@chromium.org
Link to the patchset: https://codereview.chromium.org/2937133002/#ps60001
(title: "Fix")

[commit-bot: I haz the power, 2017-06-15 03:50:43]

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-15 05:26:46]

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

[commit-bot: I haz the power, 2017-06-15 05:26:47]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[brettw, 2017-06-15 15:54:57]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2017-06-15 15:55:16]

CQ is trying da patch.

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

[brettw, 2017-06-15 18:01:35]

Description was changed from

==========
Launch browser when --memlog is specified on the command line.

This communicates a pipe name to the launched process which is propagated to
child processes. It is not yet used.

The current process topology is the profiling process launches the browser which
then runs like normal. In a later pass I will change this to have the browser
process launch the profiling process when the flag is set. I believe this
topology will be required for certain platforms. But it will require more
plumbing for "process hosts" and mojo pipes that will take more time and don't
need to block the current work.

BUG=733336
==========

to

==========
Launch browser when --memlog is specified on the command line.

This communicates a pipe name to the launched process which is propagated to
child processes. It is not yet used.

The current process topology is the profiling process launches the browser which
then runs like normal. In a later pass I will change this to have the browser
process launch the profiling process when the flag is set. I believe this
topology will be required for certain platforms. But it will require more
plumbing for "process hosts" and mojo pipes that will take more time and don't
need to block the current work.

BUG=733336
R=ajwong@chromium.org, borisv@chromium.org

Review-Url: https://codereview.chromium.org/2937133002 .
Cr-Commit-Position: refs/heads/master@{#479754}
Committed:
https://chromium.googlesource.com/chromium/src/+/1664378033de651755a3b4e46ec9...
==========

[brettw, 2017-06-15 18:01:39]

Committed patchset #4 (id:60001) manually as
1664378033de651755a3b4e46ec9690600dc8873 (presubmit successful).