|
|
Created:
3 years, 5 months ago by Ilya Sherman Modified:
3 years, 5 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[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
Patch Set 1 #Patch Set 2 : Update #includes #Patch Set 3 : Rebase #Patch Set 4 : Use SequenceTaskRunner to post Windows Mojo-bound task #Patch Set 5 : Rebase #Patch Set 6 : Try a COM thread #Patch Set 7 : Rebase #Patch Set 8 : Use COM thread for RecordStartupMetrics #Patch Set 9 : Add base:: namespace koala-fication #Patch Set 10 : For debugging: VLOG(1) -> LOG(ERROR) #Patch Set 11 : Some more logging #Patch Set 12 : Add some more logging #Patch Set 13 : Try a timeout #Patch Set 14 : The delay worked! #
Total comments: 4
Patch Set 15 : const #Messages
Total messages: 70 (54 generated)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
will wait for green bots (they're currently red)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
isherman@chromium.org changed reviewers: + gab@chromium.org
+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...
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.
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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!
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
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.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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)
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
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isherman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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 ==========
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.
The CQ bit was checked by isherman@chromium.org
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")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499888408506940, "parent_rev": "04ed6e6eff3c79d66517f2f25eb36d01318d4885", "commit_rev": "2d71d53f0142954869b02fb1a93d8c790e1743c3"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/2d71d53f0142954869b02fb1a93d...
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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? |