Issue metadata
Sign in to add a comment
|
Investigate thread priority impact on startup performance |
||||||||||||||||||||
Issue descriptionOn 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+?
,
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.
,
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).
,
Jun 1 2018
Benoit: do you want to try running with this?
,
Jun 1 2018
Sure, assigning the bug to me.
,
Jul 13
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.
,
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
,
Jul 18
,
Jul 18
Experiment started here: cl/204502314
,
Aug 1
The NextAction date has arrived: 2018-08-01
,
Aug 7
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).
,
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
,
Aug 8
,
Aug 8
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?)
,
Aug 8
Interesting! Yes, makes sense that O+ would show the biggest impact. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by yfried...@chromium.org
, May 10 2018