Ensure that TaskPriority::BACKGROUND tasks are not required to respond to user actions. |
|||||||||
Issue descriptionTaskPriority::BACKGROUND tasks can take "an arbitrarily long time to complete". https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?l=26&rcl=19a69a1389844f51c474efa446e3f22dd1fb36a8 We have plan to restrict more aggressively when they can run. To make sure that this doesn't introduce unwanted performance regressions, we need to make sure that there is no need to run TaskPriority::BACKGROUND tasks to: - Show the initial Chrome UI. - Load a page. - Send input events to a page. - Show suggestions in the omnibox. - Start a download. - Show the settings page. ...
,
Apr 12 2018
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0 commit 4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0 Author: Francois Doray <fdoray@chromium.org> Date: Mon Apr 16 15:57:48 2018 Test that first non-empty paint doesn't depend on BACKGROUND tasks. TaskPriority::BACKGROUND tasks can take "an arbitrarily long time to complete". https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?l=26&rcl=19a69a1389844f51c474efa446e3f22dd1fb36a8 We have plan to restrict more aggressively when they can run. To make sure that this doesn't introduce unwanted performance regressions, we need to make sure that there is no need to run TaskPriority::BACKGROUND tasks to respond to user actions. This CL adds a browser test to verify that the first non-empty paint can happen when no TaskPriority::BACKGROUND tasks are allowed to run. Bug: 831835 Change-Id: I4c5d4f1056d823a6c36aa1504e2508d1bc1c58b6 Reviewed-on: https://chromium-review.googlesource.com/1008640 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#550973} [add] https://crrev.com/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0/chrome/browser/no_background_tasks_browsertest.cc [modify] https://crrev.com/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0/chrome/test/BUILD.gn
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0 commit 4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0 Author: Francois Doray <fdoray@chromium.org> Date: Mon Apr 16 15:57:48 2018 Test that first non-empty paint doesn't depend on BACKGROUND tasks. TaskPriority::BACKGROUND tasks can take "an arbitrarily long time to complete". https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?l=26&rcl=19a69a1389844f51c474efa446e3f22dd1fb36a8 We have plan to restrict more aggressively when they can run. To make sure that this doesn't introduce unwanted performance regressions, we need to make sure that there is no need to run TaskPriority::BACKGROUND tasks to respond to user actions. This CL adds a browser test to verify that the first non-empty paint can happen when no TaskPriority::BACKGROUND tasks are allowed to run. Bug: 831835 Change-Id: I4c5d4f1056d823a6c36aa1504e2508d1bc1c58b6 Reviewed-on: https://chromium-review.googlesource.com/1008640 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#550973} [add] https://crrev.com/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0/chrome/browser/no_background_tasks_browsertest.cc [modify] https://crrev.com/4e84bea563809e2ef7c2ffbfdbbf83a68e9c09e0/chrome/test/BUILD.gn
,
Apr 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67 commit 2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67 Author: Francois Doray <fdoray@chromium.org> Date: Wed Apr 18 15:19:38 2018 Do not block in HostNameHandler::OnDeviceHostnamePropertyChanged(). Previously, HostNameHandler::OnDeviceHostnamePropertyChanged() could synchronously wait for machine statistics to be loaded by a TaskPriority::BACKGROUND task while being called on the main thread: chromeos::system::StatisticsProviderImpl::WaitForStatisticsLoaded() chromeos::system::StatisticsProviderImpl::GetMachineStatistic() chromeos::system::StatisticsProvider::GetEnterpriseMachineID() policy::HostnameHandler::OnDeviceHostnamePropertyChanged() chromeos::NetworkStateHandler::NotifyDefaultNetworkChanged() chromeos::internal::ShillPropertyHandler::GetIPConfigCallback() This was problematic since a TaskPriority::BACKGROUND task can take "an arbitrarily long time to complete", and the main thread is supposed to be responsibe at all times. This CL fixes the issue by continuing the work asynchronously when machine statistics are loaded rather than waiting synchronously. Bug: 831835 Change-Id: Idde6fb85f584b0110e673025f83fe53b86b657ea Reviewed-on: https://chromium-review.googlesource.com/1012182 Commit-Queue: François Doray <fdoray@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Cr-Commit-Position: refs/heads/master@{#551680} [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chrome/browser/chromeos/policy/hostname_handler.cc [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chrome/browser/chromeos/policy/hostname_handler.h [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chromeos/system/fake_statistics_provider.cc [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chromeos/system/fake_statistics_provider.h [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chromeos/system/statistics_provider.cc [modify] https://crrev.com/2e9f39badf3f13e03d3fe57a6f8f28a17fe42e67/chromeos/system/statistics_provider.h
,
May 2 2018
,
May 3 2018
,
May 3 2018
,
Jun 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3df86866ac373a1fef52d8bdf2e130090d94d21f commit 3df86866ac373a1fef52d8bdf2e130090d94d21f Author: Francois Doray <fdoray@chromium.org> Date: Tue Jun 05 16:11:12 2018 Load machine statistics with USER_BLOCKING priority on ChromeOS. The NoBackgroundTasksTest.FirstNonEmptyPaintWithoutBackgroundTasks test shows that loading machine statistics is on the critical path of loading the NTP on ChromeOS. Therefore, it must have a USER_BLOCKING priority to run as soon as possible. Bug: 831835 Change-Id: Icdcf45b7380ad270441609b4b2fb6021c8ea5933 Reviewed-on: https://chromium-review.googlesource.com/1082608 Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#564524} [modify] https://crrev.com/3df86866ac373a1fef52d8bdf2e130090d94d21f/chromeos/system/statistics_provider.cc
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/738c6fa609452a891b4c79609a7eedb2f8e042c2 commit 738c6fa609452a891b4c79609a7eedb2f8e042c2 Author: Francois Doray <fdoray@chromium.org> Date: Thu Jun 07 21:37:58 2018 Enable NoBackgroundTasksTest.FirstNonEmptyPaintWithoutBackgroundTasks on ChromeOS. The test failed because rendering the NTP was blocked on loading machine statistics, which was done from a TaskPriority::BACKGROUND task. The priority of that task was changed to TaskPriority::USER_BLOCKING in https://chromium-review.googlesource.com/c/chromium/src/+/1082608, so the test can be enabled. The test is still disabled when Mash is enabled, because the non-empty paint signal isn't fired in this case (even when background tasks aren't disabled). Bug: 831835 Change-Id: Ie847b152b057883f62bef060c6294a427980c838 Reviewed-on: https://chromium-review.googlesource.com/1082609 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#565421} [modify] https://crrev.com/738c6fa609452a891b4c79609a7eedb2f8e042c2/chrome/browser/no_background_tasks_browsertest.cc
,
Jun 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0798fbc3bc16d6fdf2805d8a16844b7dc1e59a2c commit 0798fbc3bc16d6fdf2805d8a16844b7dc1e59a2c Author: Friedrich Horschig <fhorschig@chromium.org> Date: Fri Jun 08 07:34:34 2018 Revert "Enable NoBackgroundTasksTest.FirstNonEmptyPaintWithoutBackgroundTasks on ChromeOS." This reverts commit 738c6fa609452a891b4c79609a7eedb2f8e042c2. Reason for revert: Failed Linux Chromium OS ASan LSan Tests (1) every time since being enabled. First bad build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/27807 Original change's description: > Enable NoBackgroundTasksTest.FirstNonEmptyPaintWithoutBackgroundTasks on ChromeOS. > > The test failed because rendering the NTP was blocked on loading > machine statistics, which was done from a TaskPriority::BACKGROUND > task. The priority of that task was changed to > TaskPriority::USER_BLOCKING in > https://chromium-review.googlesource.com/c/chromium/src/+/1082608, so > the test can be enabled. > > The test is still disabled when Mash is enabled, because the > non-empty paint signal isn't fired in this case (even when background > tasks aren't disabled). > > Bug: 831835 > Change-Id: Ie847b152b057883f62bef060c6294a427980c838 > Reviewed-on: https://chromium-review.googlesource.com/1082609 > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: François Doray <fdoray@chromium.org> > Cr-Commit-Position: refs/heads/master@{#565421} TBR=avi@chromium.org,fdoray@chromium.org Change-Id: I7f045845726b0075c639f4708ff183a741d60a1e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 831835 Reviewed-on: https://chromium-review.googlesource.com/1092530 Reviewed-by: Friedrich Horschig <fhorschig@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#565576} [modify] https://crrev.com/0798fbc3bc16d6fdf2805d8a16844b7dc1e59a2c/chrome/browser/no_background_tasks_browsertest.cc
,
Aug 28
,
Aug 28
I think this is high priority given the scale of TaskScheduler and how bad (but subtle) getting this wrong is (i.e. issue 878222 ).
,
Oct 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1f60a08f0eff28a3abb037074e9109281da79aa commit b1f60a08f0eff28a3abb037074e9109281da79aa Author: Francois Doray <fdoray@chromium.org> Date: Fri Oct 26 23:14:06 2018 Use USER_VISIBLE priority for tasks posted by the tracing service. We want to experiment with more aggressive throttling for BEST_EFFORT tasks. With this experiment, there can be multiple seconds delay before a BEST_EFFORT task is scheduled. Because it is currently necessary to run a BEST_EFFORT task to stop tracing, using the chrome://tracing UI is painful when the experiment is enabled. This CL fixes this issue by changing the priority of the tracing coordinator service backend TaskRunner from BEST_EFFORT to USER_VISIBLE. Note that USER_VISIBLE is the appropriate priority for a task that may affect the UI. Bug: 831835 Change-Id: Iaebeddd006f802d8f73694ef4817ea976490b6a3 Reviewed-on: https://chromium-review.googlesource.com/c/1302321 Reviewed-by: oysteine <oysteine@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#603244} [modify] https://crrev.com/b1f60a08f0eff28a3abb037074e9109281da79aa/services/tracing/coordinator.cc [modify] https://crrev.com/b1f60a08f0eff28a3abb037074e9109281da79aa/services/tracing/coordinator.h
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae33f2fd073b298a0507d7fa068c34e454b5762f commit ae33f2fd073b298a0507d7fa068c34e454b5762f Author: Francois Doray <fdoray@chromium.org> Date: Tue Oct 30 13:40:04 2018 Content: Set CompletedFirstVisuallyNonEmptyPaint() before notifying observers. Today, WebContents::CompletedFirstVisuallyNonEmptyPaint() returns false from WebContentsObserver::DidFirstVisuallyNonEmptyPaint(). This makes it hard to write a test that waits for first visually non-empty paint and another condition: class MyWebContentsObserver : public WebContentsObserver { void DidFirstVisuallyNonEmptyPaint() override { OnStateChange(); } void DidSomething() override { OnStateChange(); } void OnStateChange() { if (web_contents()->CompletedFirstVisuallyNonEmptyPaint() && web_contents()->Something()) { // This scope will never be entered from // DidFirstVisuallyNonEmptyPaint(). } } }; This CL fixes the issue by making WebContents::CompletedFirstVisuallyNonEmptyPaint() return true from WebContentsObserver::DidFirstVisuallyNonEmptyPaint(). Bug: 831835 Change-Id: Ieffd23056fa0c0aedc56dc238cc0a343a83fdcc0 Reviewed-on: https://chromium-review.googlesource.com/c/1306333 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#603881} [modify] https://crrev.com/ae33f2fd073b298a0507d7fa068c34e454b5762f/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/ae33f2fd073b298a0507d7fa068c34e454b5762f/content/browser/web_contents/web_contents_impl_unittest.cc
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d6ffeaeb31e7af2a39079983fca0ff850975b48c commit d6ffeaeb31e7af2a39079983fca0ff850975b48c Author: Francois Doray <fdoray@chromium.org> Date: Tue Oct 30 18:00:24 2018 Rename NoBackgroundTasksTest to NoBestEffortTasksTest. This is a folow-up for the rename of TaskPriority::BACKGROUND to TaskPriority::BEST_EFFORT in https://chromium-review.googlesource.com/1153461. Bug: 831835 Change-Id: Ia99f3a923622de37cb92490879d2a7b9b6b4f153 Reviewed-on: https://chromium-review.googlesource.com/c/1307673 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#603958} [rename] https://crrev.com/d6ffeaeb31e7af2a39079983fca0ff850975b48c/chrome/browser/no_best_effort_tasks_browsertest.cc [modify] https://crrev.com/d6ffeaeb31e7af2a39079983fca0ff850975b48c/chrome/test/BUILD.gn
,
Oct 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ca68f95eba2280e68e955adf62bed81b0766953 commit 4ca68f95eba2280e68e955adf62bed81b0766953 Author: Francois Doray <fdoray@chromium.org> Date: Wed Oct 31 15:04:14 2018 Test loading and painting a page from the network without BEST_EFFORT tasks. We want to experiment with scheduling features that can increase the completion time of BEST_EFFORT tasks a lot. Before we do this, we want to make sure that BEST_EFFORT tasks are not required to complete important user actions (navigation, scrolling, omnibox). Bug: 831835 Change-Id: I5b935172e0349b7f74fd6120109e1e50cb8d1653 Reviewed-on: https://chromium-review.googlesource.com/c/1307834 Commit-Queue: François Doray <fdoray@chromium.org> Reviewed-by: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#604255} [modify] https://crrev.com/4ca68f95eba2280e68e955adf62bed81b0766953/chrome/browser/no_best_effort_tasks_browsertest.cc
,
Nov 6
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by fdoray@chromium.org
, Apr 11 2018