[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

This also changes the timing with which startup metrics are recorded: Previously they were recorded ~45 seconds after startup; now they're recorded whenever the TaskScheduler schedules them to be.  This is not expected to change the values recorded to these metrics.

BUG=667892, 741816
TEST=none
R=asvitkine@chromium.org

Review-Url: https://codereview.chromium.org/2955063005
Cr-Commit-Position: refs/heads/master@{#486114}
Committed: https://chromium.googlesource.com/chromium/src/+/2d71d53f0142954869b02fb1a93d8c790e1743c3

[Ilya Sherman, 2017-06-27 19:46:28]

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

[commit-bot: I haz the power, 2017-06-27 19:46:41]

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-27 20:00:35]

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

[commit-bot: I haz the power, 2017-06-27 20:00:36]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Alexei Svitkine (slow), 2017-06-27 20:13:36]

will wait for green bots (they're currently red)

[Ilya Sherman, 2017-06-28 16:25:24]

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

[commit-bot: I haz the power, 2017-06-28 16:25:43]

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-28 18:24:13]

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

[commit-bot: I haz the power, 2017-06-28 18:24:14]

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

[Ilya Sherman, 2017-06-28 20:20:20]

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

[Ilya Sherman, 2017-06-28 20:20:20]

+gab: I'm confused by the test failures, e.g. [1].  Is this likely caused by the
lack of a ScopedTaskEnvironment() in the tests?  I'm hoping it doesn't, as that
seems like a major pain to insert into every one of the failing tests.   I'm
actually not clear how base::SequencedTaskRunnerHandle::Get() is being reached
(maybe I'm just failing to read code well?)...

[1]
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...

[gab, 2017-06-28 20:46:30]

On 2017/06/28 20:20:20, Ilya Sherman wrote:
> +gab: I'm confused by the test failures, e.g. [1].  Is this likely caused by
the
> lack of a ScopedTaskEnvironment() in the tests?  I'm hoping it doesn't, as
that
> seems like a major pain to insert into every one of the failing tests.   I'm
> actually not clear how base::SequencedTaskRunnerHandle::Get() is being reached
> (maybe I'm just failing to read code well?)...
> 
> [1]
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...

The issue is that on Windows:

shell_integration::win::GetIsPinnedToTaskbarState [0x028ECFAC+28]
	ChromeBrowserMainExtraPartsMetrics::PreProfileInit [0x0286DE7B+75]

uses mojo.

Mojo requires to run from a sequenced environnement (i.e.
SequencedTaskRunnerHandle needs to be set).

So instead of base::PostTaskWithTraits(...) you'll need to
base::CreateSequencedTaskRunnerWithTraits(...)->PostTask(...) to create an in
place sequence which will live for that task alone (+any extension of it made by
mojo through the SequencedTaskRunnerHandle).

This is indeed subtle, I'll take a note of this and discuss with the team how we
can better handle this automatically.

[Ilya Sherman, 2017-06-28 21:07:48]

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

[commit-bot: I haz the power, 2017-06-28 21:08:28]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-06-28 21:09:15]

On 2017/06/28 20:46:30, gab (slow Tue-Wed) wrote:
> On 2017/06/28 20:20:20, Ilya Sherman wrote:
> > +gab: I'm confused by the test failures, e.g. [1].  Is this likely caused by
> the
> > lack of a ScopedTaskEnvironment() in the tests?  I'm hoping it doesn't, as
> that
> > seems like a major pain to insert into every one of the failing tests.   I'm
> > actually not clear how base::SequencedTaskRunnerHandle::Get() is being
reached
> > (maybe I'm just failing to read code well?)...
> > 
> > [1]
> >
>
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.win%2...
> 
> The issue is that on Windows:
> 
> shell_integration::win::GetIsPinnedToTaskbarState [0x028ECFAC+28]
> 	ChromeBrowserMainExtraPartsMetrics::PreProfileInit [0x0286DE7B+75]
> 
> uses mojo.
> 
> Mojo requires to run from a sequenced environnement (i.e.
> SequencedTaskRunnerHandle needs to be set).
> 
> So instead of base::PostTaskWithTraits(...) you'll need to
> base::CreateSequencedTaskRunnerWithTraits(...)->PostTask(...) to create an in
> place sequence which will live for that task alone (+any extension of it made
by
> mojo through the SequencedTaskRunnerHandle).
> 
> This is indeed subtle, I'll take a note of this and discuss with the team how
we
> can better handle this automatically.

Thanks!

[commit-bot: I haz the power, 2017-06-28 22:23:08]

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

[commit-bot: I haz the power, 2017-06-28 22:23:09]

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

[Ilya Sherman, 2017-06-29 17:50:47]

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

[commit-bot: I haz the power, 2017-06-29 17:51:03]

Dry run: CQ is trying da patch.

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

[Alexei Svitkine (slow), 2017-06-29 19:17:22]

lgtm but gab's the expert

Also yay for moving random lengthy stuff from IO thread where it didn't belong
to an actually appropriate background thread.

[Ilya Sherman, 2017-06-29 19:42:16]

On 2017/06/29 19:17:22, Alexei Svitkine (slow) wrote:
> lgtm but gab's the expert
> 
> Also yay for moving random lengthy stuff from IO thread where it didn't belong
> to an actually appropriate background thread.

Yeah, definitely nice to offload that work!

I'm a bit concerned about the tests which are still failing -- it seems there
might be some sort of hidden dependency w.r.t. to the memory tracing code?  I'm
a bit stuck on that, tbh.  There are some VLOG stmts that might help to debug
it, but I'm not quite sure how to ask trybots to run with VLOG stmts enabled.

[commit-bot: I haz the power, 2017-06-29 19:57:58]

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

[commit-bot: I haz the power, 2017-06-29 19:57:59]

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

[gab, 2017-07-05 02:40:51]

On 2017/06/29 19:42:16, Ilya Sherman wrote:
> On 2017/06/29 19:17:22, Alexei Svitkine (slow) wrote:
> > lgtm but gab's the expert
> > 
> > Also yay for moving random lengthy stuff from IO thread where it didn't
belong
> > to an actually appropriate background thread.
> 
> Yeah, definitely nice to offload that work!
> 
> I'm a bit concerned about the tests which are still failing -- it seems there
> might be some sort of hidden dependency w.r.t. to the memory tracing code? 
I'm
> a bit stuck on that, tbh.  There are some VLOG stmts that might help to debug
> it, but I'm not quite sure how to ask trybots to run with VLOG stmts enabled.

Windows only failure looks like lack of COM environment? (ref.
CreateCOMStaTaskRunnerWithTraits()). robliao@ can help here)

[gab, 2017-07-05 02:41:32]

On 2017/07/05 02:40:51, gab (in-out til july17) wrote:
> On 2017/06/29 19:42:16, Ilya Sherman wrote:
> > On 2017/06/29 19:17:22, Alexei Svitkine (slow) wrote:
> > > lgtm but gab's the expert
> > > 
> > > Also yay for moving random lengthy stuff from IO thread where it didn't
> belong
> > > to an actually appropriate background thread.
> > 
> > Yeah, definitely nice to offload that work!
> > 
> > I'm a bit concerned about the tests which are still failing -- it seems
there
> > might be some sort of hidden dependency w.r.t. to the memory tracing code? 
> I'm
> > a bit stuck on that, tbh.  There are some VLOG stmts that might help to
debug
> > it, but I'm not quite sure how to ask trybots to run with VLOG stmts
enabled.
> 
> Windows only failure looks like lack of COM environment? (ref.
> CreateCOMStaTaskRunnerWithTraits()). robliao@ can help here)

Other than that, LGTM

[Ilya Sherman, 2017-07-05 18:26:55]

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

[commit-bot: I haz the power, 2017-07-05 18:27:23]

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-07-05 18:53:26]

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

[commit-bot: I haz the power, 2017-07-05 18:53:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Ilya Sherman, 2017-07-10 22:53:17]

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

[commit-bot: I haz the power, 2017-07-10 22:53:30]

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-07-10 23:06:20]

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

[commit-bot: I haz the power, 2017-07-10 23:06:22]

Dry run: 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_...)

[Ilya Sherman, 2017-07-10 23:27:10]

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

[commit-bot: I haz the power, 2017-07-10 23:27:37]

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-07-11 01:32:13]

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

[commit-bot: I haz the power, 2017-07-11 01:32:14]

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

[Ilya Sherman, 2017-07-11 02:57:28]

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

[commit-bot: I haz the power, 2017-07-11 02:57:54]

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-07-11 03:56:50]

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

[commit-bot: I haz the power, 2017-07-11 03:56:51]

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

[Ilya Sherman, 2017-07-11 05:52:41]

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

[commit-bot: I haz the power, 2017-07-11 05:52: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, 2017-07-11 06:48:17]

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

[commit-bot: I haz the power, 2017-07-11 06:48:18]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ilya Sherman, 2017-07-11 23:52:47]

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

[commit-bot: I haz the power, 2017-07-11 23:53:36]

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-07-12 01:36:52]

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

[commit-bot: I haz the power, 2017-07-12 01:36:53]

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

[Ilya Sherman, 2017-07-12 02:58:07]

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

[commit-bot: I haz the power, 2017-07-12 02:58:21]

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-07-12 04:22:17]

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

[commit-bot: I haz the power, 2017-07-12 04:22:18]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-07-12 05:05:47]

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

[commit-bot: I haz the power, 2017-07-12 05:06:12]

Dry run: CQ is trying da patch.

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

[Ilya Sherman, 2017-07-12 05:07:14]

Alexei, mind taking another look as a sanity check?  I think I finally nailed
down the last of the test failures -- I'm still not exactly sure why they were
failing, but I think I've found a workaround for now.  The code changed a bit
while I was debugging that.

[commit-bot: I haz the power, 2017-07-12 06:25:54]

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

[commit-bot: I haz the power, 2017-07-12 06:25:55]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-07-12 15:12:57]

lgtm

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:513:
base::TaskTraits background_task_traits = {
Nit: const

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:546:
base::BindOnce(&RecordStartupMetrics));
The behavior change is we're now no longer doing this 45 seconds after startup.
Can you add that info to CL description? Mention that you don't expect it to
have an effect on the metrics (since I assume that's the case.)

Probably would be good to verify that as well, once this lands - i.e. manually
check the metrics to ensure they didn't change because of this.

[Ilya Sherman, 2017-07-12 19:39:15]

Description was changed from

==========
[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

BUG=667892
TEST=none
R=asvitkine@chromium.org
==========

to

==========
[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

This also changes the timing with which startup metrics are recorded: Previously
they were recorded ~45 seconds after startup; now they're recorded whenever the
TaskScheduler schedules them to be.  This is not expected to change the values
recorded to these metrics.

BUG=667892, 741816
TEST=none
R=asvitkine@chromium.org
==========

[Ilya Sherman, 2017-07-12 19:39:47]

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:513:
base::TaskTraits background_task_traits = {
On 2017/07/12 15:12:56, Alexei Svitkine (slow) wrote:
> Nit: const

Done.

https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:546:
base::BindOnce(&RecordStartupMetrics));
On 2017/07/12 15:12:56, Alexei Svitkine (slow) wrote:
> The behavior change is we're now no longer doing this 45 seconds after
startup.
> Can you add that info to CL description? Mention that you don't expect it to
> have an effect on the metrics (since I assume that's the case.)
> 
> Probably would be good to verify that as well, once this lands - i.e. manually
> check the metrics to ensure they didn't change because of this.

Good point!  Updated the CL description and filed a follow-up bug.

[Ilya Sherman, 2017-07-12 19:40:08]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2017-07-12 19:40:08]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2955063005/#ps280001
(title: "const")

[commit-bot: I haz the power, 2017-07-12 19:40:42]

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-07-12 21:54:58]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499888408506940,
"parent_rev": "04ed6e6eff3c79d66517f2f25eb36d01318d4885", "commit_rev":
"2d71d53f0142954869b02fb1a93d8c790e1743c3"}

[commit-bot: I haz the power, 2017-07-12 21:55:10]

Description was changed from

==========
[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

This also changes the timing with which startup metrics are recorded: Previously
they were recorded ~45 seconds after startup; now they're recorded whenever the
TaskScheduler schedules them to be.  This is not expected to change the values
recorded to these metrics.

BUG=667892, 741816
TEST=none
R=asvitkine@chromium.org
==========

to

==========
[Cleanup] Migrate ChromeBrowserMainExtraPartsMetrics to use the Task Scheduler.

This also changes the timing with which startup metrics are recorded: Previously
they were recorded ~45 seconds after startup; now they're recorded whenever the
TaskScheduler schedules them to be.  This is not expected to change the values
recorded to these metrics.

BUG=667892, 741816
TEST=none
R=asvitkine@chromium.org

Review-Url: https://codereview.chromium.org/2955063005
Cr-Commit-Position: refs/heads/master@{#486114}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d71d53f0142954869b02fb1a93d...
==========

[commit-bot: I haz the power, 2017-07-12 21:55:12]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/2d71d53f0142954869b02fb1a93d...

[chromium-reviews, 2017-07-13 00:37:40]

Le mer. 12 juil. 2017 08:12, <asvitkine@chromium.org> a écrit :

> lgtm
>
>
>
>
>
>
https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
> (right):
>
>
>
https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:513:
> base::TaskTraits background_task_traits = {
> Nit: const
>

constexpr is even better for TaskTraits


>
>
https://codereview.chromium.org/2955063005/diff/260001/chrome/browser/metrics...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:546:
> base::BindOnce(&RecordStartupMetrics));
> The behavior change is we're now no longer doing this 45 seconds after
> startup. Can you add that info to CL description? Mention that you don't
> expect it to have an effect on the metrics (since I assume that's the
> case.)
>
> Probably would be good to verify that as well, once this lands - i.e.
> manually check the metrics to ensure they didn't change because of this.
>
> https://codereview.chromium.org/2955063005/
>

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

[Ilya Sherman, 2017-07-14 05:54:49]

On 2017/07/13 00:37:40, chromium-reviews wrote:
> constexpr is even better for TaskTraits

(a) https://chromium-review.googlesource.com/c/570732/
(b) Pourquoi?