Develop a better strategy for updating JumpList in slow machines |
||
Issue descriptionNotifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. We should have a better strategy to handle this case.
,
Jun 23 2017
Log JumpListUpdater functions' execution time to UMA Recent UMA data shows that the UpdateJumpList method alone in jumplist.cc takes 1+ second for more than 1% Canary users. Therefore, it's worthwhile to investigate all the pieces of this method, which is JumpListUpdater class related. BUG= 40407 , 179576 Review-Url: https://codereview.chromium.org/2818193002 Cr-Commit-Position: refs/heads/master@{#464997} Committed: https://chromium.googlesource.com/chromium/src/+/ce5d2adea8adbc9e950371ef08513de8e83d790d
,
Jun 23 2017
Time out jumplist update for very slow or busy OS Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some Chrome users. Among the 4 steps, AddTasks is in the best condition, while CommitUpdate is in the worst condition. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow or busy) with a pretty high confidence, and also there's not much we can improve on Chrome's side. For these situations, we should stop jumplist update to avoid making things worse. Code-wise, we stop the update if BeginUpdate takes more than 500 ms which is the cutoff for 99.5% Canary JumpList updates, as the following steps (old icons' deletion, new icons' creation, AddCustomCategory, CommitUpdate, etc) are very likely to take a long time too. BUG= 40407 , 179576 Review-Url: https://codereview.chromium.org/2831433003 Cr-Commit-Position: refs/heads/master@{#467009} Committed: https://chromium.googlesource.com/chromium/src/+/250b02089b81116d27bb3732ada0f8e87de02932
,
Jun 23 2017
Cancel the next 10 updates if there's a timeout in jumplist updater Notifying OS about the jumplist update consists of 4 steps in order: BeginUpdate, AddCustomCategory, AddTasks and CommitUpdate, all of which shouldn't take long. However, UMA data shows that each step can take up to tens of seconds for some users, especially for steps Beginupdate, AddCustomCategory and CommitUpdate. Since they are mostly Windows API calls, we can assume this is due to user machines (which can be very slow) with a pretty high confidence, and also there's not much we can improve on Chrome's side. Previously, we cancel the current jumplist update if BeginUpdate takes more than 500 ms, based on the assumption that the following steps will also take a long time if BeginUpdate does. The UMA data shows that this benefits some users but not the majority, which indicates that the runtime of these steps is not highly correlated. That is, AddCustomCategory and/or CommitUpdate can still take a long time even if BeginUpdate finishes quickly. To alleviate this issue, we should reduce the frequency of jumplist updates if any of those steps times out. In more details, we cancel the next 10 updates if BeginUpdate or AddCustomCategory takes more than 500 ms, or takes more than 500 ms, or CommitUpdate takes more than 1000 ms, which are the cutoffs for 99.5% JumpList update samples in Canary for the three steps, respectively. If the machine is always slow making every jumplist update time out, this penalty of cancelling the next 10 updates can alleviate this issue by 90%. BUG= 40407 , 179576 Review-Url: https://codereview.chromium.org/2868303003 Cr-Commit-Position: refs/heads/master@{#471430} Committed: https://chromium.googlesource.com/chromium/src/+/143d0a7278a1f2b6ff9173ec78e6755b7100704c
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/531f38bd194f9d8a0dd98d78a15f82947d742cec commit 531f38bd194f9d8a0dd98d78a15f82947d742cec Author: chengx <chengx@chromium.org> Date: Thu Jun 29 18:44:20 2017 Delete the right JumpList icons conditional on shell notification This CL decides which JumpList icons to delete based on the results of shell notification. That is, if the shell notification fails or times out, we delete the newly created icons as the old icons are still going to be used by the shell. Previously, we delete old icons before notifying the shell about the update. If the notification fails, the old JumpList is still used but their icons are already gone. In this case, there will be no icons showing up but the background color. By moving the icon deletion step after shell notification and introducing the dependency, this issue is fixed. In addition, this change makes it possible to cancel more updates where shell notification is detected to be slow. In more details, if AddCustomCategory API call in the shell notification process times out, we can now abort this update to avoid calling CommitUpdate which is very likely to be slow as well. This was impossible before this CL. BUG= 40407 , 716115 , 736530 Review-Url: https://codereview.chromium.org/2941323002 Cr-Commit-Position: refs/heads/master@{#483439} [modify] https://crrev.com/531f38bd194f9d8a0dd98d78a15f82947d742cec/chrome/browser/win/jumplist.cc [modify] https://crrev.com/531f38bd194f9d8a0dd98d78a15f82947d742cec/chrome/browser/win/jumplist.h
,
Jul 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a43fcb472fc064f1870d33daa8c8defe905b33d0 commit a43fcb472fc064f1870d33daa8c8defe905b33d0 Author: chengx <chengx@chromium.org> Date: Sun Jul 23 23:30:29 2017 Cancel coming updates when certain JumpList APIs time out to fail This CL records the timeout info when JumpListUpdater:: BeginUpdate or JumpListUpdater::AddCustomCategory fails. This information is used to decide if the next few updates are skipped. Previously, the jumplist update run is aborted right away when any of the two steps above fails without any runtime checked. It is possible that upon the failure, those steps have already been running for quite a long time, making them qualified to be considered as "timeout". In this case, we should reduce the update frequency via skipping the next few updates so that the slow machines have less burden. [Determine the new threshold] We drop the timeout threshold for JumpListUpdater::AddCustomCategory to 320 ms from 500 ms. This is because before this change we monitor the total execution time of AddCustomCategory for both most-visited and recently-closed categories, while now we only monitor it for the most-visited category. For most of the time, most-visited category has 5 items and recently-closed category has 3 items to process. In this case, we have logged the ratio of the duration spent adding the two categories using WinJumplist.RatioAddCategoryTime. It has a Gaussian distribution with a peak value of 1.7 and a little more samples on the right side of peak value. To make the new threshold not affect the total number of timeout updates when 500 ms was used as the threshold, the new threshold needs to separate the samples into 2 groups of a same size. From the histogram, we can see that the ratio of 1.8 does this job. Therefore, the new threshold for adding the most-visited category alone should be around 500/2.8*1.8 = 320 ms. BUG= 40407 , 736530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2965773002 Cr-Commit-Position: refs/heads/master@{#488899} [modify] https://crrev.com/a43fcb472fc064f1870d33daa8c8defe905b33d0/chrome/browser/win/jumplist.cc
,
Jul 24 2017
I've made all the possible improvement I could think of, so I decide to close this bug.
,
Sep 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b40ba4fe84de066c4d00dda095ce0a46a6af110 commit 6b40ba4fe84de066c4d00dda095ce0a46a6af110 Author: Xi Cheng <chengx@chromium.org> Date: Mon Sep 11 17:20:51 2017 Adjust the number of update notifications to skip for slow jumplist updates This CL reduces the number of update notifications to skip when a previous update was too slow from 10 to 2. This is an adjustment after fixing crbug/761063, where jumplist receives lots of fake update notifications from TopSites. Before that fix, jumplist receives 50k notifications from TabRestore service and 170k notifications from TopSites per day in Canary. After that fix, jumplist receives only 5k update notifications from TopSites, which is only 3% of the original amount. Therefore, this CL ensures that the user will get a jumplist update after closing 3 tabs if a previous update was too slow. This is a good compromise between alleviating the machine under heavy load and keeping jumplist useful. Bug: 736530 Cq-Include-Trybots: master.tryserver.chromium.win:win10_chromium_x64_rel_ng Change-Id: I3ab3195bf0bbd4a965d15dba546100adcbd4fb59 Reviewed-on: https://chromium-review.googlesource.com/636324 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Greg Thompson <grt@chromium.org> Cr-Commit-Position: refs/heads/master@{#500944} [modify] https://crrev.com/6b40ba4fe84de066c4d00dda095ce0a46a6af110/chrome/browser/win/jumplist.cc |
||
►
Sign in to add a comment |
||
Comment 1 by chengx@chromium.org
, Jun 23 2017