Move SetupMetrics() to PreMainMessageLoopRun

Move SetupMetrics() to PreMainMessageLoopRun

This allows the Browser Task Scheduler to initialize during
CreateThreads since SetupMetrics() ultimately requires use of the
blocking pool. The blocking pool has to be redirected by the Browser
Task Scheduler before this point

BUG=662052
Committed: https://crrev.com/1892c914719015f06c135127fd1dfd13dec39d33
Cr-Commit-Position: refs/heads/master@{#435664}

[robliao, 2016-11-22 01:36:42]

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

[commit-bot: I haz the power, 2016-11-22 01:37:40]

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-11-22 03:08:10]

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

[commit-bot: I haz the power, 2016-11-22 03:08:11]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-11-22 18:03:13]

robliao@chromium.org changed reviewers:
+ asvitkine@chromium.org

[fdoray, 2016-11-22 18:21:15]

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

[fdoray, 2016-11-22 18:21:15]

lgtm with nit

https://codereview.chromium.org/2514373003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2514373003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1415: // blocking pool, which won't be
redirected until the CreateThreads phase
, which is disabled until the ...

[Alexei Svitkine (slow), 2016-11-22 18:39:38]

asvitkine@chromium.org changed reviewers:
+ manzagop@chromium.org

[Alexei Svitkine (slow), 2016-11-22 18:39:38]

One consequence of this is that all the stability book-keeping that happens in
InitializeMetricsState() in metrics service will be done later.

+manzagop for his thoughts on that

[robliao, 2016-11-22 20:39:59]

https://codereview.chromium.org/2514373003/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2514373003/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1415: // blocking pool, which won't be
redirected until the CreateThreads phase
On 2016/11/22 18:21:15, fdoray wrote:
> , which is disabled until the ...

Done.

[manzagop (departed), 2016-11-22 20:53:46]

On 2016/11/22 18:39:38, Alexei Svitkine (slow) wrote:
> One consequence of this is that all the stability book-keeping that happens in
> InitializeMetricsState() in metrics service will be done later.
> 
> +manzagop for his thoughts on that

+1 to the idea that we'd like 1) the initial stability report 2) beginning of
UMA crash detector coverage to happen as early as possible.

IIUC this removes UMA crash coverage from CreateThreads and
BrowserThreadsStarted which doesn't seem ideal.

I wonder if these 2 things rely on the blocking pool, or if we could split them
out to keep them where they are.

[robliao, 2016-11-22 21:56:02]

On 2016/11/22 20:53:46, manzagop wrote:
> On 2016/11/22 18:39:38, Alexei Svitkine (slow) wrote:
> > One consequence of this is that all the stability book-keeping that happens
in
> > InitializeMetricsState() in metrics service will be done later.
> > 
> > +manzagop for his thoughts on that
> 
> +1 to the idea that we'd like 1) the initial stability report 2) beginning of
> UMA crash detector coverage to happen as early as possible.
> 
> IIUC this removes UMA crash coverage from CreateThreads and
> BrowserThreadsStarted which doesn't seem ideal.
> 
> I wonder if these 2 things rely on the blocking pool, or if we could split
them
> out to keep them where they are.

The Chrome Metrics Service Client provides the Metrics Service.

Currently, the metric services client posts to the underlying
SequencedWorkerPool for the following things:

1) NetworkMetricsProvider initialization
(NetworkMetricsProvider::ProbeWifiPHYLayerProtocol)
2) Removing the Previous Metrics File (BrowserMetrics.pma)
3) Removing the Previous Crashpad Metrics File (CrashpadMetrics.pma)

The SequencedWorkerPool (via BlockingPool) is also referenced in

1) FileMetricsProvider initialization
2) WatcherMetricsProviderWin initialization
3) AntiVirusMetrics initialization

Is metrics willing to do a two phase initialization? Can the metrics file
deletes be deferred?

[Alexei Svitkine (slow), 2016-11-22 22:01:22]

Splitting initialization to keep the stability stuff early seems reasonable
to me. I suspect file deletes can indeed be deferred - but we should be
careful to not introduce double reporting (i.e. if we report the data in a
file, we should make sure to keep the window tight between that and
deleting it so it doesn't get reported again if there's a crash).

On Tue, Nov 22, 2016 at 4:56 PM, <robliao@chromium.org> wrote:

> On 2016/11/22 20:53:46, manzagop wrote:
> > On 2016/11/22 18:39:38, Alexei Svitkine (slow) wrote:
> > > One consequence of this is that all the stability book-keeping that
> happens
> in
> > > InitializeMetricsState() in metrics service will be done later.
> > >
> > > +manzagop for his thoughts on that
> >
> > +1 to the idea that we'd like 1) the initial stability report 2)
> beginning of
> > UMA crash detector coverage to happen as early as possible.
> >
> > IIUC this removes UMA crash coverage from CreateThreads and
> > BrowserThreadsStarted which doesn't seem ideal.
> >
> > I wonder if these 2 things rely on the blocking pool, or if we could
> split
> them
> > out to keep them where they are.
>
> The Chrome Metrics Service Client provides the Metrics Service.
>
> Currently, the metric services client posts to the underlying
> SequencedWorkerPool for the following things:
>
> 1) NetworkMetricsProvider initialization
> (NetworkMetricsProvider::ProbeWifiPHYLayerProtocol)
> 2) Removing the Previous Metrics File (BrowserMetrics.pma)
> 3) Removing the Previous Crashpad Metrics File (CrashpadMetrics.pma)
>
> The SequencedWorkerPool (via BlockingPool) is also referenced in
>
> 1) FileMetricsProvider initialization
> 2) WatcherMetricsProviderWin initialization
> 3) AntiVirusMetrics initialization
>
> Is metrics willing to do a two phase initialization? Can the metrics file
> deletes be deferred?
>
>
>
> https://codereview.chromium.org/2514373003/
>

-- 
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.

[robliao, 2016-11-22 22:06:45]

On 2016/11/22 22:01:22, Alexei Svitkine (slow) wrote:
> Splitting initialization to keep the stability stuff early seems reasonable
> to me. I suspect file deletes can indeed be deferred - but we should be
> careful to not introduce double reporting (i.e. if we report the data in a
> file, we should make sure to keep the window tight between that and
> deleting it so it doesn't get reported again if there's a crash).
> 
> On Tue, Nov 22, 2016 at 4:56 PM, <mailto:robliao@chromium.org> wrote:
> 
> > On 2016/11/22 20:53:46, manzagop wrote:
> > > On 2016/11/22 18:39:38, Alexei Svitkine (slow) wrote:
> > > > One consequence of this is that all the stability book-keeping that
> > happens
> > in
> > > > InitializeMetricsState() in metrics service will be done later.
> > > >
> > > > +manzagop for his thoughts on that
> > >
> > > +1 to the idea that we'd like 1) the initial stability report 2)
> > beginning of
> > > UMA crash detector coverage to happen as early as possible.
> > >
> > > IIUC this removes UMA crash coverage from CreateThreads and
> > > BrowserThreadsStarted which doesn't seem ideal.
> > >
> > > I wonder if these 2 things rely on the blocking pool, or if we could
> > split
> > them
> > > out to keep them where they are.
> >
> > The Chrome Metrics Service Client provides the Metrics Service.
> >
> > Currently, the metric services client posts to the underlying
> > SequencedWorkerPool for the following things:
> >
> > 1) NetworkMetricsProvider initialization
> > (NetworkMetricsProvider::ProbeWifiPHYLayerProtocol)
> > 2) Removing the Previous Metrics File (BrowserMetrics.pma)
> > 3) Removing the Previous Crashpad Metrics File (CrashpadMetrics.pma)
> >
> > The SequencedWorkerPool (via BlockingPool) is also referenced in
> >
> > 1) FileMetricsProvider initialization
> > 2) WatcherMetricsProviderWin initialization
> > 3) AntiVirusMetrics initialization
> >
> > Is metrics willing to do a two phase initialization? Can the metrics file
> > deletes be deferred?
> >
> >
> >
> > https://codereview.chromium.org/2514373003/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Which stability stuff is absolutely necessary?

[manzagop (departed), 2016-11-24 15:51:56]

On 2016/11/22 22:06:45, robliao wrote:
> On 2016/11/22 22:01:22, Alexei Svitkine (slow) wrote:
> > Splitting initialization to keep the stability stuff early seems reasonable
> > to me. I suspect file deletes can indeed be deferred - but we should be
> > careful to not introduce double reporting (i.e. if we report the data in a
> > file, we should make sure to keep the window tight between that and
> > deleting it so it doesn't get reported again if there's a crash).
> > 
> > On Tue, Nov 22, 2016 at 4:56 PM, <mailto:robliao@chromium.org> wrote:
> > 
> > > On 2016/11/22 20:53:46, manzagop wrote:
> > > > On 2016/11/22 18:39:38, Alexei Svitkine (slow) wrote:
> > > > > One consequence of this is that all the stability book-keeping that
> > > happens
> > > in
> > > > > InitializeMetricsState() in metrics service will be done later.
> > > > >
> > > > > +manzagop for his thoughts on that
> > > >
> > > > +1 to the idea that we'd like 1) the initial stability report 2)
> > > beginning of
> > > > UMA crash detector coverage to happen as early as possible.
> > > >
> > > > IIUC this removes UMA crash coverage from CreateThreads and
> > > > BrowserThreadsStarted which doesn't seem ideal.
> > > >
> > > > I wonder if these 2 things rely on the blocking pool, or if we could
> > > split
> > > them
> > > > out to keep them where they are.
> > >
> > > The Chrome Metrics Service Client provides the Metrics Service.
> > >
> > > Currently, the metric services client posts to the underlying
> > > SequencedWorkerPool for the following things:
> > >
> > > 1) NetworkMetricsProvider initialization
> > > (NetworkMetricsProvider::ProbeWifiPHYLayerProtocol)
> > > 2) Removing the Previous Metrics File (BrowserMetrics.pma)
> > > 3) Removing the Previous Crashpad Metrics File (CrashpadMetrics.pma)
> > >
> > > The SequencedWorkerPool (via BlockingPool) is also referenced in
> > >
> > > 1) FileMetricsProvider initialization
> > > 2) WatcherMetricsProviderWin initialization
> > > 3) AntiVirusMetrics initialization
> > >
> > > Is metrics willing to do a two phase initialization? Can the metrics file
> > > deletes be deferred?
> > >
> > >
> > >
> > > https://codereview.chromium.org/2514373003/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Which stability stuff is absolutely necessary?

Actually it looks like I'm full of it. UMA crash coverage actually starts in
PreMainMessageLoopRunImpl:
https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?l=...

So this CL doesn't change the crash coverage, it only delays the initial
stability log.

[Alexei Svitkine (slow), 2016-11-24 21:32:48]

Generally LGTM % comment below and assuming manzagop@'s also OK from stability
metrics POV.

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1416: SetupMetrics();
Do we have stats on how much later this happens than the previous location?

Reason I ask is that from this moment the next upload is scheduled after the
given timer interval. So if this part of startup happens later, it will also
affect timing of metrics reports. If the time difference is negligible then it's
probably fine. Otherwise, we may want to decrease the initial timer to avoid
introducing a change in metrics reporting time as a result of this.

[gab, 2016-11-25 13:16:08]

gab@chromium.org changed reviewers:
+ gab@chromium.org, jochen@chromium.org

[gab, 2016-11-25 13:16:09]

Thanks, @jochen for OWNERS on chrome_browser_main.cc

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1416: SetupMetrics();
On 2016/11/24 21:32:48, Alexei Svitkine (slow) wrote:
> Do we have stats on how much later this happens than the previous location?
> 
> Reason I ask is that from this moment the next upload is scheduled after the
> given timer interval. So if this part of startup happens later, it will also
> affect timing of metrics reports. If the time difference is negligible then
it's
> probably fine. Otherwise, we may want to decrease the initial timer to avoid
> introducing a change in metrics reporting time as a result of this.

We do not have specific stats but as you can see @
https://docs.google.com/document/d/1MN7I9f_7lUthZkmDaTBHsCtOUB8ApiJ8y-AIbs6ub...
:

the biggest chunk of startup is PreMainMessageLoopRun and things following it.

The only thing that runs between the end of PreCreateThreadsImpl and the
beginning of PreMainMessageLoopRunImpl is content::CreateThreads and
content::BrowserThreadsCreated :
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?q=Pr...

I've never seen those as problematic in startup traces.

The timing difference is therefore negligible IMO.

[gab, 2016-11-25 13:16:53]

On 2016/11/25 13:16:09, gab wrote:
> Thanks, @jochen for OWNERS on chrome_browser_main.cc

(oh and lgtm from me of course :) -- sending OWNERS review on behalf of Rob
during American holidays)

> 
>
https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
> chrome/browser/chrome_browser_main.cc:1416: SetupMetrics();
> On 2016/11/24 21:32:48, Alexei Svitkine (slow) wrote:
> > Do we have stats on how much later this happens than the previous location?
> > 
> > Reason I ask is that from this moment the next upload is scheduled after the
> > given timer interval. So if this part of startup happens later, it will also
> > affect timing of metrics reports. If the time difference is negligible then
> it's
> > probably fine. Otherwise, we may want to decrease the initial timer to avoid
> > introducing a change in metrics reporting time as a result of this.
> 
> We do not have specific stats but as you can see @
>
https://docs.google.com/document/d/1MN7I9f_7lUthZkmDaTBHsCtOUB8ApiJ8y-AIbs6ub...
> :
> 
> the biggest chunk of startup is PreMainMessageLoopRun and things following it.
> 
> The only thing that runs between the end of PreCreateThreadsImpl and the
> beginning of PreMainMessageLoopRunImpl is content::CreateThreads and
> content::BrowserThreadsCreated :
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?q=Pr...
> 
> I've never seen those as problematic in startup traces.
> 
> The timing difference is therefore negligible IMO.

[manzagop (departed), 2016-11-25 15:20:34]

I'm ok with the change to initial metrics log. lgtm

[jochen (gone - plz use gerrit), 2016-11-28 08:36:13]

lgtm

[robliao, 2016-11-28 14:39:55]

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

[commit-bot: I haz the power, 2016-11-28 14:40:09]

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

[robliao, 2016-11-28 14:41:48]

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2514373003/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1416: SetupMetrics();
On 2016/11/25 13:16:08, gab wrote:
> On 2016/11/24 21:32:48, Alexei Svitkine (slow) wrote:
> > Do we have stats on how much later this happens than the previous location?
> > 
> > Reason I ask is that from this moment the next upload is scheduled after the
> > given timer interval. So if this part of startup happens later, it will also
> > affect timing of metrics reports. If the time difference is negligible then
> it's
> > probably fine. Otherwise, we may want to decrease the initial timer to avoid
> > introducing a change in metrics reporting time as a result of this.
> 
> We do not have specific stats but as you can see @
>
https://docs.google.com/document/d/1MN7I9f_7lUthZkmDaTBHsCtOUB8ApiJ8y-AIbs6ub...
> :
> 
> the biggest chunk of startup is PreMainMessageLoopRun and things following it.
> 
> The only thing that runs between the end of PreCreateThreadsImpl and the
> beginning of PreMainMessageLoopRunImpl is content::CreateThreads and
> content::BrowserThreadsCreated :
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?q=Pr...
> 
> I've never seen those as problematic in startup traces.
> 
> The timing difference is therefore negligible IMO.

Agreed. A few things do occur at CreateThreads and PreCreateThreads, but those
are mostly quick operations like instantiations.

[commit-bot: I haz the power, 2016-11-28 15:18:06]

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

[commit-bot: I haz the power, 2016-11-28 15:18:06]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-12-01 17:59:01]

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

[commit-bot: I haz the power, 2016-12-01 17:59:27]

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-12-01 18:53:01]

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

[commit-bot: I haz the power, 2016-12-01 18:53:03]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-12-01 18:54:54]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-12-01 18:54:55]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org,
manzagop@chromium.org, jochen@chromium.org, asvitkine@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2514373003/#ps40001
(title: "Rebase https://crrev.com/2517363002")

[commit-bot: I haz the power, 2016-12-01 18:55:13]

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-12-01 18:59:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480618494872700,
"parent_rev": "9ab00750adeed55451be6390b2bc12fa3d00da5c", "commit_rev":
"1d9a764cc3f2b190b7548a05f304e6a347c2138c"}

[commit-bot: I haz the power, 2016-12-01 19:00:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-01 19:03:06]

Description was changed from

==========
Move SetupMetrics() to PreMainMessageLoopRun

This allows the Browser Task Scheduler to initialize during
CreateThreads since SetupMetrics() ultimately requires use of the
blocking pool. The blocking pool has to be redirected by the Browser
Task Scheduler before this point

BUG=662052
==========

to

==========
Move SetupMetrics() to PreMainMessageLoopRun

This allows the Browser Task Scheduler to initialize during
CreateThreads since SetupMetrics() ultimately requires use of the
blocking pool. The blocking pool has to be redirected by the Browser
Task Scheduler before this point

BUG=662052

Committed: https://crrev.com/1892c914719015f06c135127fd1dfd13dec39d33
Cr-Commit-Position: refs/heads/master@{#435664}
==========

[commit-bot: I haz the power, 2016-12-01 19:03:07]

Patchset 3 (id:??) landed as
https://crrev.com/1892c914719015f06c135127fd1dfd13dec39d33
Cr-Commit-Position: refs/heads/master@{#435664}