New issue
Advanced search Search tips

Issue 736530 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug



Sign in to add a comment

Develop a better strategy for updating JumpList in slow machines

Project Member Reported by chengx@chromium.org, Jun 23 2017

Issue description

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.

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.

 

Comment 1 by chengx@chromium.org, Jun 23 2017

Actually quite a number of improvements have been made to alleviate this issue. Given that the core JumpList bug is going to be closed soon, I am filing this bug to track any future improvements in handling this issue.

I will also copy-n-paste all related CLs previously landed down below.

Comment 2 by chengx@chromium.org, 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

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

Comment 4 by chengx@chromium.org, 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
Project Member

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

Project Member

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

Comment 7 by chengx@chromium.org, Jul 24 2017

Status: Fixed (was: Started)
I've made all the possible improvement I could think of, so I decide to close this bug.
Project Member

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