Stack sampling profiler: run the profiler in the GPU process on trunk builds

Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958
Committed: https://crrev.com/832321f529ba13840b22c45da67e47b00ef987b3
Cr-Commit-Position: refs/heads/master@{#424185}

[Mike Wittman, 2016-09-27 17:38:00]

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

[commit-bot: I haz the power, 2016-09-27 17:38:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike Wittman, 2016-09-27 17:48:40]

wittman@chromium.org changed reviewers:
+ isherman@chromium.org

[Mike Wittman, 2016-09-27 17:48:41]

Hi Ilya, I'd appreciate a review on this from the metrics perspective before
sending on to other owners.

[Ilya Sherman, 2016-09-28 01:02:15]

Thanks, Mike.  LGTM % nits:

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:33: bool
ExecutingInBrowserProcess() {
nit: I'd prepend an "Is" to this name.  And, please add a brief doc string =)

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:70: DCHECK(configuration_ ==
PROFILE_FROM_COMMAND_LINE);
nit: DCHECK_EQ

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:47: const std::string process_type,
nit: Did you mean to pass by const-reference?   We generally don't pass by const
copy.

[Mike Wittman, 2016-09-28 19:34:26]

Thanks!

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:33: bool
ExecutingInBrowserProcess() {
On 2016/09/28 01:02:15, Ilya Sherman wrote:
> nit: I'd prepend an "Is" to this name.  And, please add a brief doc string =)

Changed to IsBrowserProcess. IsExecutingInBrowserProcess sounded strange to me
since the implicit subject is a thread, and the thread itself is not really
relevant here. Added a doc string.

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:70: DCHECK(configuration_ ==
PROFILE_FROM_COMMAND_LINE);
On 2016/09/28 01:02:15, Ilya Sherman wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2360143006/diff/60001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:47: const std::string process_type,
On 2016/09/28 01:02:15, Ilya Sherman wrote:
> nit: Did you mean to pass by const-reference?   We generally don't pass by
const
> copy.

Oops, yes that was the intent. Done.

[Mike Wittman, 2016-09-28 19:41:16]

Description was changed from

==========
Stack sampling profiler: run the profiler in the GPU process on trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
trunk builds. Also update the configuration variations to support future
synthetic field trials on canary and dev releases.

BUG=517958
==========

to

==========
Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit
trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958
==========

[Mike Wittman, 2016-09-28 19:49:21]

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

[commit-bot: I haz the power, 2016-09-28 19:49:52]

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, 2016-09-28 21:38:02]

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

[commit-bot: I haz the power, 2016-09-28 21:38:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Mike Wittman, 2016-09-29 18:39:30]

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

[commit-bot: I haz the power, 2016-09-29 18:39:50]

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, 2016-09-29 18:42:35]

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

[commit-bot: I haz the power, 2016-09-29 18:42:36]

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

[Mike Wittman, 2016-09-29 23:37:29]

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

[commit-bot: I haz the power, 2016-09-29 23:37:47]

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, 2016-09-30 00:35:13]

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

[commit-bot: I haz the power, 2016-09-30 00:35:14]

Dry run: This issue passed the CQ dry run.

[Mike Wittman, 2016-09-30 00:50:01]

wittman@chromium.org changed reviewers:
+ sievers@chromium.org, thestig@chromium.org

[Mike Wittman, 2016-09-30 00:50:01]

Hi Lei and Daniel, please take a look.

This change enables the UMA sampling profiler for GPU process startup in trunk
builds, so we can start exercising the code in this process during test runs and
developer builds. This is the preliminary step before starting to enable in
canary and dev builds (see http://crbug.com/650869).

thestig: *
sievers: chrome/gpu

[Lei Zhang, 2016-09-30 01:00:32]

Where does this actually run the profiler for Windows 64 bit trunk builds?

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:936: const char kStartStackProfiler[]          
 = "start-stack-profiler";
Is this a permanent-ish command line switch?

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.cc:17: static
base::LazyInstance<StackSamplingConfiguration>::Leaky g_configuration =
No need for static inside an anonymous namespace.

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.cc:72: // This is a child process.
The kStartStackProfiler switch passed by the
|kStartStackProfiler|

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:66: // Chrome processes. In the
child processes it is alwasys
typo: "always"

[Mike Wittman, 2016-09-30 02:13:28]

On 2016/09/30 01:00:32, Lei Zhang wrote:
> Where does this actually run the profiler for Windows 64 bit trunk builds?

At the Start call in the ChromeContentGpuClient constructor. This is gated by
StackSamplingConfiguration::IsProfilerEnabledForCurrentProcess, which checks for
the command line flag supplied by the browser process.

That flag in turn is supplied based on the Windows 64 bit trunk enable
configuration computed in the StackSamplingConfiguration instance in the browser
process.

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:936: const char kStartStackProfiler[]          
 = "start-stack-profiler";
On 2016/09/30 01:00:32, Lei Zhang wrote:
> Is this a permanent-ish command line switch?

Yes.

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.cc:17: static
base::LazyInstance<StackSamplingConfiguration>::Leaky g_configuration =
On 2016/09/30 01:00:32, Lei Zhang wrote:
> No need for static inside an anonymous namespace.

Done.

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.cc:72: // This is a child process.
The kStartStackProfiler switch passed by the
On 2016/09/30 01:00:32, Lei Zhang wrote:
> |kStartStackProfiler|

Done.

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2360143006/diff/120001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:66: // Chrome processes. In the
child processes it is alwasys
On 2016/09/30 01:00:32, Lei Zhang wrote:
> typo: "always"

Done.

[Lei Zhang, 2016-09-30 02:19:31]

On 2016/09/30 02:13:28, Mike Wittman wrote:
> On 2016/09/30 01:00:32, Lei Zhang wrote:
> > Where does this actually run the profiler for Windows 64 bit trunk builds?
> 
> At the Start call in the ChromeContentGpuClient constructor. This is gated by
> StackSamplingConfiguration::IsProfilerEnabledForCurrentProcess, which checks
for
> the command line flag supplied by the browser process.
> 
> That flag in turn is supplied based on the Windows 64 bit trunk enable
> configuration computed in the StackSamplingConfiguration instance in the
browser
> process.

I'm looking at StackSamplingConfiguration::GenerateConfiguration() and I don't
see anything about Windows or 64-bit specifically. What am I missing?

[Mike Wittman, 2016-09-30 14:55:13]

On 2016/09/30 02:19:31, Lei Zhang wrote:
> On 2016/09/30 02:13:28, Mike Wittman wrote:
> > On 2016/09/30 01:00:32, Lei Zhang wrote:
> > > Where does this actually run the profiler for Windows 64 bit trunk builds?
> > 
> > At the Start call in the ChromeContentGpuClient constructor. This is gated
by
> > StackSamplingConfiguration::IsProfilerEnabledForCurrentProcess, which checks
> for
> > the command line flag supplied by the browser process.
> > 
> > That flag in turn is supplied based on the Windows 64 bit trunk enable
> > configuration computed in the StackSamplingConfiguration instance in the
> browser
> > process.
> 
> I'm looking at StackSamplingConfiguration::GenerateConfiguration() and I don't
> see anything about Windows or 64-bit specifically. What am I missing?

There's an ifdef in IsProfilerSupported which is called from
GenerateConfiguration.

[Lei Zhang, 2016-09-30 15:39:42]

Thanks, lgtm

[no sievers, 2016-09-30 18:13:01]

sievers@chromium.org changed reviewers:
+ posciak@chromium.org
- sievers@chromium.org

[Mike Wittman, 2016-10-04 22:27:32]

Hi Pawel, can you please take a look at the chrome/gpu part of this change?

This was assigned to sievers@, but he is no longer in chrome/gpu OWNERS.

[Pawel Osciak, 2016-10-06 09:37:05]

On 2016/10/04 22:27:32, Mike Wittman wrote:
> Hi Pawel, can you please take a look at the chrome/gpu part of this change?
> 
> This was assigned to sievers@, but he is no longer in chrome/gpu OWNERS.

Sorry, I own *arc*.* in chrome/gpu, but I'm afraid I'm not familiar enough with
chrome_content_gpu_client.cc.

[Mike Wittman, 2016-10-10 14:53:17]

wittman@chromium.org changed reviewers:
+ wuchengli@chromium.org
- posciak@chromium.org

[Mike Wittman, 2016-10-10 14:54:31]

On 2016/10/06 09:37:05, Pawel Osciak wrote:
> On 2016/10/04 22:27:32, Mike Wittman wrote:
> > Hi Pawel, can you please take a look at the chrome/gpu part of this change?
> > 
> > This was assigned to sievers@, but he is no longer in chrome/gpu OWNERS.
> 
> Sorry, I own *arc*.* in chrome/gpu, but I'm afraid I'm not familiar enough
with
> chrome_content_gpu_client.cc.

Wu-cheng, can you review this change?

[wuchengli, 2016-10-10 15:33:09]

On 2016/10/10 14:54:31, Mike Wittman wrote:
> On 2016/10/06 09:37:05, Pawel Osciak wrote:
> > On 2016/10/04 22:27:32, Mike Wittman wrote:
> > > Hi Pawel, can you please take a look at the chrome/gpu part of this
change?
> > > 
> > > This was assigned to sievers@, but he is no longer in chrome/gpu OWNERS.
> > 
> > Sorry, I own *arc*.* in chrome/gpu, but I'm afraid I'm not familiar enough
> with
> > chrome_content_gpu_client.cc.
> 
> Wu-cheng, can you review this change?
I'm the same as Pawel. I know *arc* files but not chrome_content_gpu_client.cc.
Can you find someone to review chrome_content_gpu_client.cc? I can give a
rubberstamp after someone reviews it.

[Mike Wittman, 2016-10-10 17:15:59]

On 2016/10/10 15:33:09, wuchengli wrote:
> On 2016/10/10 14:54:31, Mike Wittman wrote:
> > On 2016/10/06 09:37:05, Pawel Osciak wrote:
> > > On 2016/10/04 22:27:32, Mike Wittman wrote:
> > > > Hi Pawel, can you please take a look at the chrome/gpu part of this
> change?
> > > > 
> > > > This was assigned to sievers@, but he is no longer in chrome/gpu OWNERS.
> > > 
> > > Sorry, I own *arc*.* in chrome/gpu, but I'm afraid I'm not familiar enough
> > with
> > > chrome_content_gpu_client.cc.
> > 
> > Wu-cheng, can you review this change?
> I'm the same as Pawel. I know *arc* files but not
chrome_content_gpu_client.cc.
> Can you find someone to review chrome_content_gpu_client.cc? I can give a
> rubberstamp after someone reviews it.

Looks like the other chrome/gpu owner, piman, is OOO and from git history there
doesn't seem to be another good GPU reviewer for chrome_content_gpu_client.cc.

Based on discussion with thestig I will submit this change under his top-level
approval.

[Mike Wittman, 2016-10-10 17:16:33]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2016-10-10 17:16:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2360143006/#ps140001
(title: "address comments")

[commit-bot: I haz the power, 2016-10-10 17:16:47]

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, 2016-10-10 18:18:59]

Description was changed from

==========
Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit
trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958
==========

to

==========
Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit
trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958
==========

[commit-bot: I haz the power, 2016-10-10 18:19:00]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-10 18:22:55]

Description was changed from

==========
Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit
trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958
==========

to

==========
Stack sampling profiler: run the profiler in the GPU process on Windows 64 bit
trunk builds

Configure the sampling profiler to run in both the browser and GPU process on
Windows 64 bit trunk builds. Also update the configuration variations to support
future synthetic field trials on canary and dev releases.

BUG=517958

Committed: https://crrev.com/832321f529ba13840b22c45da67e47b00ef987b3
Cr-Commit-Position: refs/heads/master@{#424185}
==========

[commit-bot: I haz the power, 2016-10-10 18:22:56]

Patchset 8 (id:??) landed as
https://crrev.com/832321f529ba13840b22c45da67e47b00ef987b3
Cr-Commit-Position: refs/heads/master@{#424185}