New issue
Advanced search Search tips

Issue 831835 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Feature

Blocked on:
issue 878222

Blocking:
issue 588745
issue 839110
issue 839397
issue 902441



Sign in to add a comment

Ensure that TaskPriority::BACKGROUND tasks are not required to respond to user actions.

Project Member Reported by fdoray@chromium.org, Apr 11 2018

Issue description

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:

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

 

Comment 1 by fdoray@chromium.org, Apr 11 2018

Description: Show this description

Comment 2 by gab@chromium.org, Apr 12 2018

Blocking: 588745
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
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

Project Member

Comment 5 by bugdroid1@chromium.org, 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

Comment 6 by gab@chromium.org, May 2 2018

Blocking: 839110

Comment 7 by gab@chromium.org, May 3 2018

Blocking: 839397

Comment 8 by gab@chromium.org, May 3 2018

Labels: -Type-Bug Type-Feature
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Project Member

Comment 11 by bugdroid1@chromium.org, 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

Blockedon: 878222
Labels: -Pri-3 Pri-1
I think this is high priority given the scale of TaskScheduler and how bad (but subtle) getting this wrong is (i.e.  issue 878222 ).
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by bugdroid1@chromium.org, 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

Project Member

Comment 16 by bugdroid1@chromium.org, 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

Project Member

Comment 17 by bugdroid1@chromium.org, 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

Blocking: 902441

Sign in to add a comment