Stack sampling profiler: enable at 10% for GPU process on canary

Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869
Committed: https://crrev.com/37380b7afac6486840437172edc13a66bb4cd81b
Cr-Commit-Position: refs/heads/master@{#424923}

[Mike Wittman, 2016-10-12 19:02:51]

Description was changed from

==========
Stack sampling profiler: enable at 10% on canary

BUG=650869
==========

to

==========
Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869
==========

[Mike Wittman, 2016-10-12 19:02:51]

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

[Mike Wittman, 2016-10-12 19:04:11]

Hi Ilya, can you do a preliminary review for this?

[Ilya Sherman, 2016-10-12 20:52:31]

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:162: }
Hmm, it looks like you're re-implementing the field trial mechanism.  Why?

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:71: // sum to 100.
Why are weights required to sum to 100?  What if you want to run a 33%/33%/33%
trial?  (This restriction isn't present in the //base variations code, which is
why I'm asking why it's needed here.)

[Mike Wittman, 2016-10-12 21:50:41]

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:162: }
On 2016/10/12 20:52:31, Ilya Sherman wrote:
> Hmm, it looks like you're re-implementing the field trial mechanism.  Why?

The profiler can't use the standard field trial mechanism because it runs before
the field trials are initialized.

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:71: // sum to 100.
On 2016/10/12 20:52:31, Ilya Sherman wrote:
> Why are weights required to sum to 100?  What if you want to run a 33%/33%/33%
> trial?  (This restriction isn't present in the //base variations code, which
is
> why I'm asking why it's needed here.)

It's not strictly needed, but is a small sanity check. If I need a configuration
that doesn't work with this in the future I'll remove the constraint and DCHECK
at that point.

[Ilya Sherman, 2016-10-12 22:16:17]

LGTM % documentation nits

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:162: }
On 2016/10/12 21:50:41, Mike Wittman wrote:
> On 2016/10/12 20:52:31, Ilya Sherman wrote:
> > Hmm, it looks like you're re-implementing the field trial mechanism.  Why?
> 
> The profiler can't use the standard field trial mechanism because it runs
before
> the field trials are initialized.

Ah, makes sense -- thanks for the reminder.  Is this documented somewhere
already alongside the code?  If so, I didn't spot it.

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:71: // sum to 100.
On 2016/10/12 21:50:41, Mike Wittman wrote:
> On 2016/10/12 20:52:31, Ilya Sherman wrote:
> > Why are weights required to sum to 100?  What if you want to run a
33%/33%/33%
> > trial?  (This restriction isn't present in the //base variations code, which
> is
> > why I'm asking why it's needed here.)
> 
> It's not strictly needed, but is a small sanity check. If I need a
configuration
> that doesn't work with this in the future I'll remove the constraint and
DCHECK
> at that point.

Got it, that makes sense.  Would be nice to briefly document somewhere that this
is done as a config sanity-check =)

[Mike Wittman, 2016-10-12 23:00:32]

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.cc:162: }
On 2016/10/12 22:16:17, Ilya Sherman wrote:
> On 2016/10/12 21:50:41, Mike Wittman wrote:
> > On 2016/10/12 20:52:31, Ilya Sherman wrote:
> > > Hmm, it looks like you're re-implementing the field trial mechanism.  Why?
> > 
> > The profiler can't use the standard field trial mechanism because it runs
> before
> > the field trials are initialized.
> 
> Ah, makes sense -- thanks for the reminder.  Is this documented somewhere
> already alongside the code?  If so, I didn't spot it.

Not explicitly. Added to the comment on GetSyntheticFieldTrial.

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

https://codereview.chromium.org/2409953004/diff/20001/chrome/common/stack_sam...
chrome/common/stack_sampling_configuration.h:71: // sum to 100.
On 2016/10/12 22:16:17, Ilya Sherman wrote:
> On 2016/10/12 21:50:41, Mike Wittman wrote:
> > On 2016/10/12 20:52:31, Ilya Sherman wrote:
> > > Why are weights required to sum to 100?  What if you want to run a
> 33%/33%/33%
> > > trial?  (This restriction isn't present in the //base variations code,
which
> > is
> > > why I'm asking why it's needed here.)
> > 
> > It's not strictly needed, but is a small sanity check. If I need a
> configuration
> > that doesn't work with this in the future I'll remove the constraint and
> DCHECK
> > at that point.
> 
> Got it, that makes sense.  Would be nice to briefly document somewhere that
this
> is done as a config sanity-check =)

Done.

[Mike Wittman, 2016-10-12 23:02:18]

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

[Mike Wittman, 2016-10-12 23:02:18]

Hi Lei, please take a look.

[Lei Zhang, 2016-10-12 23:05:59]

lgtm

[Mike Wittman, 2016-10-12 23:34:49]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2016-10-12 23:34:50]

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/2409953004/#ps40001
(title: "update comments")

[commit-bot: I haz the power, 2016-10-12 23:35:31]

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-12 23:45:14]

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

[commit-bot: I haz the power, 2016-10-12 23:45:15]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mike Wittman, 2016-10-12 23:49:41]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2016-10-12 23:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2409953004/#ps60001
(title: "clang fix")

[commit-bot: I haz the power, 2016-10-12 23:50:15]

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-12 23:57:01]

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

[commit-bot: I haz the power, 2016-10-12 23:57:03]

Exceeded global retry quota

[Mike Wittman, 2016-10-13 00:44:29]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2016-10-13 00:44:38]

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-13 00:50:00]

Description was changed from

==========
Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869
==========

to

==========
Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869
==========

[commit-bot: I haz the power, 2016-10-13 00:50:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-13 00:52:15]

Description was changed from

==========
Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869
==========

to

==========
Stack sampling profiler: enable at 10% for GPU process on canary

Also updates the control group to have the profiler enabled in the browser
process (the current steady state).

BUG=650869

Committed: https://crrev.com/37380b7afac6486840437172edc13a66bb4cd81b
Cr-Commit-Position: refs/heads/master@{#424923}
==========

[commit-bot: I haz the power, 2016-10-13 00:52:16]

Patchset 4 (id:??) landed as
https://crrev.com/37380b7afac6486840437172edc13a66bb4cd81b
Cr-Commit-Position: refs/heads/master@{#424923}