Enable startup profiling by Win x64 stack sampling profiler

Enable startup profiling by Win x64 stack sampling profiler

Collects stack samples at 10 Hz for 30 seconds at startup. Uploading of
samples to UMA is controlled by a Finch experiment.

This change moves to a callback-based mechanism for communicating
completed profiles to the metrics provider to avoid profiles lingering
in memory when reporting is disabled.

BUG=464929
Committed: https://crrev.com/ae2a08ac68bcb4000d1ddf391dbf19aa285cc48f
Cr-Commit-Position: refs/heads/master@{#324175}

[Mike Wittman, 2015-03-23 19:32:42]

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

[Mike Wittman, 2015-03-24 00:38:06]

Patchset #3 (id:40001) has been deleted

[Mike Wittman, 2015-03-24 00:42:18]

PTAL

This is the follow-on CL to enable the profiler.

cpu: base, chrome/browser
isherman: components/metrics

[Ilya Sherman, 2015-03-24 04:02:04]

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:25: bool
IsSamplingProfilingReportingEnabled() {
nit: Doc string, please.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:116: }
nit: Please leave a blank line here.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:136:
weak_factory_.GetWeakPtr()));
Note that weak pointers are only safe to read on their originating thread.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:37: static const char
kReportProfilesGroupName[];
nit: If these are only used for tests, please mark them as protected, and expose
them for tests in a test-specific subclass.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:44: const
std::vector<base::StackSamplingProfiler::Profile>& profiles);
Can this be moved to an anonymous namespace in the implementation file?

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:49:
std::vector<base::StackSamplingProfiler::Profile> pending_profiles_;
Could you please instead post a task to the thread that the class lives on?  I
don't think metrics providers have any thread-safety guarantees...

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:22: namespace
{
nit: Please leave a blank line here.

[Mike Wittman, 2015-03-24 18:37:39]

https://codereview.chromium.org/1029653002/diff/60001/base/profiler/stack_sam...
File base/profiler/stack_sampling_profiler_unittest.cc (right):

https://codereview.chromium.org/1029653002/diff/60001/base/profiler/stack_sam...
base/profiler/stack_sampling_profiler_unittest.cc:389:
TEST(StackSamplingProfilerTest, MAYBE_DefaultCallback) {
This test and the following one need to be run independent of each other and the
tests above due to setting global state. I'm planning to use the test
serialization support in https://crrev.com/1027993002 if and when it lands.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:25: bool
IsSamplingProfilingReportingEnabled() {
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> nit: Doc string, please.

Done.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:116: }
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> nit: Please leave a blank line here.

Done.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:136:
weak_factory_.GetWeakPtr()));
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> Note that weak pointers are only safe to read on their originating thread.

Ah, didn't realize that. Updated to post a task back to the metrics provider's
thread.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:37: static const char
kReportProfilesGroupName[];
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> nit: If these are only used for tests, please mark them as protected, and
expose
> them for tests in a test-specific subclass.

Done.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:44: const
std::vector<base::StackSamplingProfiler::Profile>& profiles);
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> Can this be moved to an anonymous namespace in the implementation file?

Done.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:49:
std::vector<base::StackSamplingProfiler::Profile> pending_profiles_;
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> Could you please instead post a task to the thread that the class lives on?  I
> don't think metrics providers have any thread-safety guarantees...

Done.

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1029653002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:22: namespace
{
On 2015/03/24 04:02:03, Ilya Sherman wrote:
> nit: Please leave a blank line here.

Done.

[Ilya Sherman, 2015-03-24 21:07:55]

Thanks!  LGTM % nits:

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:26: void
IgnoreCompletedProfiles(
nit: Doc string, please.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:13: #include
"base/synchronization/lock.h"
nit: No longer needed.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:40: static const char
kReportProfilesGroupName[];
Optional: Actually, an easier option for testing might be to expose a protected
method "virtual bool IsEnabled()" (or a more detailed name, if you prefer).  The
production implementation could test the field trial state, whereas the test
could just return true or false as desired.  That way, the test doesn't need to
worry about Finch integration at all.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:49:
scoped_refptr<base::MessageLoopProxy> message_loop,
nit: Forward-declare the MessageLoopProxy class?

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:26: class
CallStackProfileMetricsProviderTest : public testing::Test {
nit: I believe that test fixtures are supposed to be defined outside of the
anonymous namespace, because something something name mangling.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:40:
field_trial_list_.reset();
nit: No need to call this explicitly, unless it needs to happen before the test
fixture's destructor is called for some reason.

[Mike Wittman, 2015-03-24 22:40:59]

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.h (right):

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:13: #include
"base/synchronization/lock.h"
On 2015/03/24 21:07:55, Ilya Sherman wrote:
> nit: No longer needed.

Done.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:40: static const char
kReportProfilesGroupName[];
On 2015/03/24 21:07:55, Ilya Sherman wrote:
> Optional: Actually, an easier option for testing might be to expose a
protected
> method "virtual bool IsEnabled()" (or a more detailed name, if you prefer). 
The
> production implementation could test the field trial state, whereas the test
> could just return true or false as desired.  That way, the test doesn't need
to
> worry about Finch integration at all.

Hmm, I'm not enthusiastic about the impact of that approach on the interface.
Providing a method to determine the enable state implies that state varies
per-object in the implementation, when in fact it's global.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.h:49:
scoped_refptr<base::MessageLoopProxy> message_loop,
On 2015/03/24 21:07:55, Ilya Sherman wrote:
> nit: Forward-declare the MessageLoopProxy class?

Done.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right):

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:26: class
CallStackProfileMetricsProviderTest : public testing::Test {
On 2015/03/24 21:07:55, Ilya Sherman wrote:
> nit: I believe that test fixtures are supposed to be defined outside of the
> anonymous namespace, because something something name mangling.

Yes, that sounds familiar.

https://codereview.chromium.org/1029653002/diff/80001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider_unittest.cc:40:
field_trial_list_.reset();
On 2015/03/24 21:07:55, Ilya Sherman wrote:
> nit: No need to call this explicitly, unless it needs to happen before the
test
> fixture's destructor is called for some reason.

Removed. Also moved the SetUp contents into the constructor, which is preferred
according to the Google Test FAQ.

[Mike Wittman, 2015-03-25 21:59:46]

Patchset #6 (id:120001) has been deleted

[cpu_(ooo_6.6-7.5), 2015-03-31 23:39:07]

https://codereview.chromium.org/1029653002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1029653002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:587: sampling_profiler_.Start();
so what is our plan?  we are branching this week, so I was expecting to see this
under some guard. If it is only 24 hrs we can leave it and then remove it with
another CL.

[Mike Wittman, 2015-04-01 00:13:12]

In patch set 7 I've rebased this CL against my readability CL in
https://codereview.chromium.org/1030923002.

If you review the readability CL first, please review patch set 7. Otherwise
please take a look at patch set 6. Thanks!

https://codereview.chromium.org/1029653002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1029653002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:587: sampling_profiler_.Start();
On 2015/03/31 23:39:07, cpu wrote:
> so what is our plan?  we are branching this week, so I was expecting to see
this
> under some guard. If it is only 24 hrs we can leave it and then remove it with
> another CL.

We can land this after the branch point.

[cpu_(ooo_6.6-7.5), 2015-04-07 00:36:52]

lgtm

go ahead and carry your profiling for a few days, then please disable it.

https://codereview.chromium.org/1029653002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1029653002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:731:
base::StackSamplingProfiler::SamplingParams
this function does not need to be exposed to the interface (in the .h file) so
please put this function in as a freestanding function in this file, ideally in
the unnamed namespace.

[Mike Wittman, 2015-04-07 01:17:10]

On 2015/04/07 00:36:52, cpu wrote:
> go ahead and carry your profiling for a few days, then please disable it.

Can we enable this in a way that provides a continuous flow of data, even if
only from a subset of clients? For example, enabling just on canary/dev and/or
enabling the collection via Finch? It's fairly likely we'll need to make
additional tweaks to the client implementation over time as we're bringing up
the full client/server processing pipeline.

[cpu_(ooo_6.6-7.5), 2015-04-07 17:48:41]

Do a writeup how you see this being permanently enabled, in which cases, what
population etc. Then we can talk with Pawliger and Laforge.

[Mike Wittman, 2015-04-07 21:21:55]

On 2015/04/07 17:48:41, cpu wrote:
> Do a writeup how you see this being permanently enabled, in which cases, what
> population etc. Then we can talk with Pawliger and Laforge.

Will do. I'll submit this now with the understanding that we'll either remove it
or have a plan for how it can be on but restricted.

[Mike Wittman, 2015-04-07 21:56:52]

Ilya, I've made a small change in patch set 10 to avoid storing profiles if the
Finch config is disabled, and clear profiles we've processed. Can you take
another quick look before I submit?

[Ilya Sherman, 2015-04-07 22:01:23]

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:155:
pending_profiles_.clear();
Whoops!  Good catch!

Could you please add a test that would catch this error if it were to resurface?

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:186: if
(IsSamplingProfilingReportingEnabled())
nit: Please use curlies, since the body of the stmt spans multiple lines.

[Mike Wittman, 2015-04-07 22:38:14]

Patchset #11 (id:240001) has been deleted

[Mike Wittman, 2015-04-07 22:40:29]

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:155:
pending_profiles_.clear();
On 2015/04/07 22:01:22, Ilya Sherman wrote:
> Whoops!  Good catch!
> 
> Could you please add a test that would catch this error if it were to
resurface?

Done.

https://codereview.chromium.org/1029653002/diff/220001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:186: if
(IsSamplingProfilingReportingEnabled())
On 2015/04/07 22:01:22, Ilya Sherman wrote:
> nit: Please use curlies, since the body of the stmt spans multiple lines.

Done.

[Ilya Sherman, 2015-04-07 22:42:19]

LGTM, thanks.

[Mike Wittman, 2015-04-07 22:42:46]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-04-07 22:42:47]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1029653002/#ps260001
(title: "add test")

[commit-bot: I haz the power, 2015-04-07 22:43:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029653002/260001

[Mike Wittman, 2015-04-07 22:43:56]

The CQ bit was unchecked by wittman@chromium.org

[Mike Wittman, 2015-04-08 00:54:40]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2015-04-08 00:55:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029653002/260001

[commit-bot: I haz the power, 2015-04-08 02:20:46]

Committed patchset #11 (id:260001)

[commit-bot: I haz the power, 2015-04-08 02:21:34]

Patchset 11 (id:??) landed as
https://crrev.com/ae2a08ac68bcb4000d1ddf391dbf19aa285cc48f
Cr-Commit-Position: refs/heads/master@{#324175}