New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 841804 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 8
Cc:
EstimatedDays: ----
NextAction: 2018-08-01
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Investigate thread priority impact on startup performance

Project Member Reported by dskiba@chromium.org, May 10 2018

Issue description

On Android Chrome sets DISPLAY priority to its UI thread (which is Android's main thread), see https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?l=1201

DISPLAY priority corresponds to -4 (Process.THREAD_PRIORITY_DISPLAY).

The change was done in context of  issue 164651 .

However, on O main thread seems to have priority of -10, which seems to be ActivityManagerService.TOP_APP_PRIORITY_BOOST.

Maybe we should stop setting DISPLAY on O+?
 
Cc: lizeb@chromium.org agrieve@chromium.org
So we're inadvertently lowering the priority of the main thread on O? that seems backwards.

DO we even need to set the priority? Could we not trust android to default it correctly?

Comment 2 by lizeb@chromium.org, May 14 2018

After a bit of digging in the logs, I traced this back to:
https://chromiumcodereview.appspot.com/11880014

From a comment there:

// Threads on linux/android may inherit their priority from the thread
// where they were created. This sets all threads to the default.
// TODO(epenner): Move thread priorities to base. ( crbug.com/170549 )

That would explain why we first lower the priority of our thread, then raise it?
Though I don't think this would apply to the UI thread, as it is not created by Chrome in the first place.

If my understanding is correct, then we probably shouldn't set the priority ourselves.

Comment 3 by dskiba@chromium.org, May 31 2018

Asked Android guys about -10 on O, and apparently that was done to reduce jank. The boost is only applied when the app is in the foreground, though, i.e. once it goes to the background main thread priority goes back to the default (0).
Benoit: do you want to try running with this? 

Comment 5 by lizeb@google.com, Jun 1 2018

Cc: -lizeb@chromium.org
Owner: lizeb@chromium.org
Status: Assigned (was: Available)
Sure, assigning the bug to me.
Owner: skyos...@chromium.org
Status: Started (was: Assigned)
Agreed to take this over from Benoit. Looks like the code in the framework will eventually reset us back to TOP_APP_PRIORITY_BOOST (when OOM scores are adjusted), but the lower priority might still hurt us early in the process lifetime.

I'm thinking of conditionally resetting the niceness to -4 unless it was already lower and seeing if it moves anything in UMA.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/805ae7576195a1d1c93d83751945c39951925776

commit 805ae7576195a1d1c93d83751945c39951925776
Author: Sami Kyostila <skyostil@chromium.org>
Date: Wed Jul 18 15:58:20 2018

Adjust logic for UI thread priority

This patch adjusts the logic for UI thread priority:

1) The priority is only increased to DISPLAY unless it was lower to
   begin with. Recent versions of Android (O+) automatically set the top
   activity's UI thread's niceness to TOP_APP_PRIORITY_BOOST (-10), so
   by setting the priority to DISPLAY (-4) we were in fact lowering the
   priority.

2) The priority is increased earlier in the start-up flow to minimize
   the chances of descheduling.

We also add a field trial for the first change to evaluate its impact.

BUG= 841804 
TEST=Manual inspection

Change-Id: I3ec70042be86f2b88bb1ae3a2dd8cba7d87eb807
Reviewed-on: https://chromium-review.googlesource.com/1136648
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576079}
[modify] https://crrev.com/805ae7576195a1d1c93d83751945c39951925776/content/browser/browser_main_loop.cc
[modify] https://crrev.com/805ae7576195a1d1c93d83751945c39951925776/content/public/common/content_features.cc
[modify] https://crrev.com/805ae7576195a1d1c93d83751945c39951925776/content/public/common/content_features.h

NextAction: 2018-08-01
Experiment started here: cl/204502314
The NextAction date has arrived: 2018-08-01
Data from UMA: https://uma.googleplex.com/p/chrome/variations/?sid=b286e568c610608d39f5d557480c45c5

Does not seem statistically significant, but suggests a 8% improvement in scroll smoothness in the 99th percentile as well as a 8% speedup in time to first contentful paint at startup (also 99th%ile).
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c928b2c7544bc84e25f379567bef49b487ed429

commit 6c928b2c7544bc84e25f379567bef49b487ed429
Author: Sami Kyostila <skyostil@chromium.org>
Date: Wed Aug 08 11:03:44 2018

Remove experiment for overriding UI thread priority

Remove the experiment for overriding UI thread priority, making the new
behavior the default.

The data from UMA[1] shows a (statistically insignificant) 8%
improvement in Event.Latency.ScrollBegin.Touch.TimeToScrollUpdateSwapBegin2
(99%ile) and and an 8% drop in
Startup.Android.Cold.TimeToFirstContentfulPaint.Tabbed (also 99%ile).

BUG= 841804 

[1] https://uma.googleplex.com/p/chrome/variations/?sid=b286e568c610608d39f5d557480c45c5

Change-Id: I55fe266c0101cf4b120c098e90d08bfde7e158a1
Reviewed-on: https://chromium-review.googlesource.com/1165345
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581517}
[modify] https://crrev.com/6c928b2c7544bc84e25f379567bef49b487ed429/content/browser/browser_main_loop.cc
[modify] https://crrev.com/6c928b2c7544bc84e25f379567bef49b487ed429/content/public/common/content_features.cc
[modify] https://crrev.com/6c928b2c7544bc84e25f379567bef49b487ed429/content/public/common/content_features.h

Status: Fixed (was: Started)
Filtering for O shows an even bigger difference: https://uma.googleplex.com/p/chrome/variations/?sid=6e5c82753d313a6f45e170e9e8554af5

(presumably cause that's where we were negatively impacting things?)
Interesting! Yes, makes sense that O+ would show the biggest impact.

Sign in to add a comment