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

Issue 40407 link

Starred by 76 users

Windows 7 Jumplisticon massive performance bug

Reported by powap...@gmail.com, Apr 5 2010

Issue description

Chrome Version       : 5.0.370.0 (43585) and possibly *all* version "5"
URLs (if applicable) : *all*
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:

     Chrome v5 running in *VISTA* sp2 compatabilty: OK


What steps will reproduce the problem?

1. On a *windows 7* machine:
Open multiple tabs in chrome, preferably more than 30
2. Close all the tabs


What is the expected result?

Closing all tabs should not result in chrome being un-usable for extended 
period of time.
Expected a more reasonable cpu and io usage closing tabs.


What happens instead?

Watch CPU usage max out and massive I/O in folders "chrome\user 
data\default\jumplisticon" to "chrome\user data\default\jumplisticonOLD"


Please provide any additional information below. Attach a screenshot if
possible.

Workaround is to run in vista mode as mentioned wherein jumplisticons 
feature is not available and thus no performance issue.

 
Showing comments 44 - 143 of 143 Older
Even if it WAS intended, it still qualifies as a performance bug, as it can bring the whole browser to a screeching halt, and users (this thread/issue) have seen it do so.

JumpList updates are just far too frequent.

And for those who are concerned that google doesn't care about this issue:

They've been actively investigating it - albeit not with the urgency of a critical security issue - it's just a hard problem. 

Why is it hard? Let me try to explain:

The JumpList is updated in an asynchronous manner, when something needs to update the JumpList, it calls JumpList::PostRunUpdate.

PostRunUpdate in turn "posts" a message in the file thread, which says "please update the JumpList", and the file thread "puts" the message in line with all the other things it needs to do (save that file you're downloading to disk, load your preferences from a file, etc...).

Whatever needed to update the JumpList then moves along with whatever it was doing, without waiting for the JumpList to be updated.

This mechanism is critical, or the entire browser might have to wait for something silly, like uploading a tiny file.

The problem is that many things can all ask for an update, without knowing whether anybody else is ALSO asking for an update, or even whether itself has just asked for an update.

And once the file thread gets to a message, it can't tell it apart from any other message, or even whether the update is necessary.

This makes it quite hard to tell why any specific JumpListUpdate was posted, after the fact.

That in itself isn't too big of a deal, but Chrome is a huge system, and components can interact in strange/unpredictable ways. Thus, a lot of investigative work is necessary.


Mr.Dawson, would you say that that's a fair characterization?
Looking at comment #44, could this be also (part of) the reason my file dialogs are hanging? Kinda weird if it is.

I'm also suspecting the JumpList update goes crazy each time I close my browser or a window with lots of tabs where it keeps updating the JumpList while all tabs close instead of just the last few. Which should be understandable that it slows down closing of the browser for no good reason.

Question however: Does the JumpList update need to be done with tons of temporary files and not just in-memory? Considering the files are very small, it shouldn't require much memory to do it and would certainly be faster with less bottlenecking. I honestly don't know how JumpList works on the OS side, so sorry if the files are actually required by it.

Comment 46 by Deleted ...@, Apr 20 2015

Been having the same problem although my JumpListIcons folder only has 22 items my ssd performance drops significantly.

I did notice that these are actually image files (rename to .png or .bmp) if that adds something to the discussion/solution?
#45:

File dialogs hanging? When you say that, what do you mean? 

I'm not sure if it's related, but I think I know what you're talking about..... I'll hit save (CTRL + S) and nothing happens for a good minute or so.

Could you try and get an ETW trace?

#46:

That's interesting.....it also makes sense, even though I don't have much experience with Windows Shell programming.

It sortof helped, as it stirred me to look through the source again and I noticed that the function that writes the icons to disk, IconUtil::CreateIconFileFromImageFamily, does so with: base::ImportantFileWriter::WriteFileAtomically, which does a ::FlushFileBuffers after writing - which is a big performance no-no.


I seem to remember Bruce saying something, about the call to ::FlushFileBuffers, somewhere else..... 

Why do we need to do it atomically? Sounds like something is wrong!

Ah, here's the mention:

https://codereview.chromium.org/965503002/
@comment 47
I have already posted about it on more specific bug reports on that regard. And I do mean that the dialog takes a long time to appear even though there's absolutely no reason for it to delay outside of maybe what was covered in comment 44. I can commonly open the same URL in different browser and save or upload the file there before Chrome responds when the delay happens, and it seems to coincide with Chrome using lots of HDD for no perceivable reason (closed a window with lots of tabs maybe?).

There's old profiler save in https://code.google.com/p/chromium/issues/detail?id=348386#c1 but I could try to make another one again.
I too have this problem with Chromium. Normally, browser hangup wouldn't be that big of a problem, but it causes the whole system to hang, to the point where the system has to be forced to shut down. Not even a REISUB will get the machine to reboot. 
Well, I don't expect REISUB/REISUO to work on Windows, because however nice a feature, it's not a Windows feature.

Unless you mean a MANUALLY_INITIATED_CRASH, which is a different key combination entirely.
@49, I wouldn't be surprised if it's loading a DLL - it should still give some indication.
Here's a new profiler snapshot.. It's just 60 seconds of data when the browser was being _Very_ slow. I opened two save dialogs at start of the period which responded slightly before the end of the period.
profile.7z
240 KB Download
Looking at the debugging instructions, it seems ETW trace is some other thing though? I'll try to see if I can figure it out.
Since chrome dev team never wants to fix bugs that really bug users.

I worked around the issue by changing access permissions on both folders 'JumpListIconsOld' and 'JumpListIcons'.
Right-click on the folder choose Properties. In Security tab. Select your user name than deny Read and Write permissions.
Note: This will disable jump-list icons for the chrome entry in windows on the taskbar.

I've not had a hard-drive grind aka hangs since then. I wish I did this while my SSD was still alive. Chrome killed it in 2 years. TWO years!

JumplistIcons.png
19.7 KB View Download
> Since chrome dev team never wants to fix bugs that really bug users.

Hey! They do fix a lot of the bugs that really bug users! With a few notable exceptions (#147!!!!), they're hard at work. They do just as good (if not better) a job as any other large C/C++ project.  Like every other large C/C++ project, I get the sense that they're understaffed.
Cc: hichris...@gmail.com
Labels: -Pri-3 -Cr-UI Pri-2 Hotlist-Slow
Was this ever dealt with? Moving back to priority 2 & adding to Hotlist-Slow (hopefully someone'll look at it).
No.
I have a patch which I hope to land in the next week or so which should reduce the cost of this, especially in the pathological cases. I'd hoped to better understand those cases before trying to land the patch but that seems unlikely so I'm proceeding anyway.
Project Member

Comment 61 by bugdroid1@chromium.org, Jun 11 2015

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

commit c539ec64212e2a08e4b15c4ed29072e1d4f9fcab
Author: brucedawson <brucedawson@chromium.org>
Date: Thu Jun 11 19:45:09 2015

Delay jumplist updates, filter redundant updates, and instrument updates.

Filter out redundant jumplist updates by ignoring provably pointless
updates and posting the update task with a delay to make more updates
redundant. A delay of 3500 ms was chosen because it should be short
enough to not noticeably affect the freshness of the jumplist, but
long enough to let more redundant requests arrive. A delay of 3500 ms
also guarantees (assuming a typical update time of ~500 ms) that the
browser will not spend a majority of its time updating the jumplist.

This change also adds Chrome tracing events to JumpList::PostRunUpdate
and JumpList::DeferredRunUpdate for testing and easier diagnosis of
update request storms.

There have been reports for years of excess updates to the Windows 7+
jumplist icons. One source of these problems is that
JumpList::PostRunUpdate may be called multiple times, queueing up a
long series of redundant updates.

A modest version of this was caught on my machine on the new-tab-page
where certain icons that fail to download trigger update requests
every time.

Another version of this has been inferred from ETW trace data from a
customer. This showed JumpList::RunUpdateOnFileThread being called
about 70 times over a 33 s period, with no gaps in-between. The
simplest explanation for this behavior is queueing of redundant
update requests. Note that this data shows that each JumpListUpdate
call consumes almost half a second, on a beefy machine with lots of
memory, many cores, and an SSD.

In that customer's pathological scenario we will now go from having
70+ jumplist updates to having just one, and we will be able to
better investigate the cause.

Other scenarios that have been reported include closing all tabs
during browser shutdown but I have not reproduced this.

It is worth separately investigating why JumpListUpdate is so
expensive (a FlushFileBuffers call for each icon is a problem) and
why these redundant calls are happening (NTP is one known cause,
the customer's calls appear to originate from syncer::GetSessionName)
but coalescing the redundant calls is still worthwhile.

While this change will not fix the related problem of ending up
with thousands of jumplist icon files it should reduce the number of
files created.

TEST=This change can be tested by going to chrome://tracing, clicking
Record, checking 'browser' only, and then clicking record. Then close a few
tabs in quick succession and wait five seconds before stopping the trace.
A search of the trace for JumpList should show a JumpList::PostRunUpdate
event for each tab that was closed, and a JumpList::DeferredRunUpdate for
the actual update that occurs ~3.5 s after the last
JumpList::PostRunUpdate.  Bug 498987  tracks adding automated tests.
BUG= 40407 , 179576 , 498987 

Review URL: https://codereview.chromium.org/965503002

Cr-Commit-Position: refs/heads/master@{#334015}

[modify] http://crrev.com/c539ec64212e2a08e4b15c4ed29072e1d4f9fcab/chrome/browser/jumplist_win.cc
[modify] http://crrev.com/c539ec64212e2a08e4b15c4ed29072e1d4f9fcab/chrome/browser/jumplist_win.h

Cc: kavvaru@chromium.org
Labels: Needs-Feedback
Verifed the fix as per the Test mentioned in comment#61

Tested the issue on windows 7 using chrome dev version 45.0.2431.0 with the belowsteps

1.Go to Chrome://tracing
2.Checked the browser option and click on record
3.closed some 5 tabs
4.Stop the recording
5.Not able to see the JumpList::PostRunUpdate event in trace

Please find the attached screen cast and confirm anything missed here.
Provide us the detailed steps to verify from QA end.
Thanks,
Trace.mp4
1.4 MB Download

Comment 65 by ajha@chromium.org, Aug 17 2015

Cc: ajha@chromium.org
Labels: -Needs-Feedback TE-Verified-46.0.2485.0 TE-Verified-M46
Owner: brucedaw...@chromium.org
Status: Assigned
Tested on Windows-7, chrome version:46.0.2485.0  as per the test steps given in C#61.

Didn't observe any 'JumpList::PostRunUpdate' in trace after 1-2s of the last tab close, which is expected as per C#64, hence adding the verified labels.

brucedawson@: If there is no further work to be done on this, could you please close the issue.

Thank you!

Comment 66 by q2w3q...@gmail.com, Aug 17 2015

I beg to differ. 
As an end user I still(v46.0.2478.0) notice hdd thrashing cached by Jumplists behavior. I have no traces to offer but I monitor where chrome makes this writes plus I've put chrome's cache in a different  drive. It happens just  when chrome runs its Jumplist routines. For users who keep many tabs open chrome still seems to torture their hdds.

I want to say a thanks to Bruce for his work but this issue still exists IMHO.
I have heard other reports that this issue is not fixed. The cause of the continuing problems is not known. Since I cannot reproduce the problem locally I am dependent on getting traces from customers.

An ETW trace would probably let us understand this issue. The process for recording these is much easier, especially as of Chrome 46. In order to record a great ETW trace of Chrome:

1) Go to chrome://flags and search for trace-export. Enable "Enable exporting of tracing events to ETW" and relaunch Chrome
2) Go to https://github.com/google/UIforETW/releases and download etwpackage.zip. Unzip it. Run etwpackage\bin\UIforETW.exe. Wait for Windows Performance Toolkit installs.
3) From UIforETW go to the Settings dialog and in Chrome tracing categories check 'browser' and 'toplevel'.
4) Click Start Tracing. Reproduce the problem. type Ctrl+Win+C to save the trace to disk. Then type in a description of what happened and when (i.e.; "Closed a couple of tabs with Ctrl+W, hard disk went crazy, waited ~5 seconds and then recorded the trace.") and share the trace.

You can contact me at brucedawson at chromium.org in order to share the trace or for more information on how to record traces.

Comment 68 by q2w3q...@gmail.com, Aug 19 2015

Alright, I sent you some traces at you inbox.
Chromium	46.0.2486.0 (Developer Build) (32-bit)
Revision	26b95242b8853564a5c7d406d2ba2c6d2d359096-refs/heads/master@{#343662}
OS	Windows 7
Blink	537.36 (@200639)

I'm seeing the 3.5-5 second wait, but then a slew of continuous JumpListIcons activity that lasts 45-50 seconds or more.

Attached is a screen capture recording of Process Hacker open looking at the Properties of the parent chrome.exe process with the Handles tab open.

I had about 62 tabs opened, then opened 3 more for this experiment. I set the region of the screen to record (includes part of the path column and the handle column), started the recording and then: 

closed 2 of the latest three tabs
opened another two links into 2 new tabs
closed all 3 of those tabs

I was finished closing tabs by around 1 minute into the video. 

The JumpListIcons activity lasted for ~1.5 minutes after I stopped the recording. I thought it was over, but it had just paused for a few seconds.

I know it's not as good as a trace, but I'm using tools I already have on this box.
JumpListIcons chromium.mp4
1.9 MB Download
The ETW traces I received showed a few interesting points:

1) JumpList updates are being throttled, but not enough, and they still appear to be triggered multiple times from closing one tab, for unknown reasons. I may tweak the throttling code but I may also need some chrome traces to try to understand why they are triggered so often.
2) The jumplist disk I/O seems to be very inefficient on some disks. 95% of the time was spent in disk flush calls which should not be needed.
3) The ETW trace did not properly record all of the desired information so I am going to make some tweaks to improve that.

I do want to point out that the traces clearly show that the disks are going idle between updates, for a couple of seconds, so that is progress, but it is not enough progress.

Anyone want to send in a Chrome trace with the categories browser, toplevel, and toplevel.flow enabled? Start tracing, close a tab, wait ~5 seconds, then stop tracing?

The initial fix (https://codereview.chromium.org/965503002) resolved some aspects of this issue, but not all.

Thanks to some customer traces that I received and some inspired guesses by a coworker I have now identified one cause of the excessive JumpList updates. It appears that Chrome has a bug which causes it to do one JumpList update for each top-level window. Some power users have 10+ top-level Chrome windows and therefore end up being hit by this pretty hard. This will be fixed, which will help those users significantly, although I do not currently have a fix or an ETA.

I also have a tentative fix that will make the JumpList updates less expensive, which should benefit all Chrome users.

> Some power users have 10+ top-level Chrome windows and therefore end up being hit by this pretty hard.

*snickers*
Project Member

Comment 73 by bugdroid1@chromium.org, Oct 1 2015

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

commit dfa983eaa8492758ccf761676820f763220f5c9a
Author: brucedawson <brucedawson@chromium.org>
Date: Thu Oct 01 01:04:13 2015

Skip flushes on jumplist create for 10x+ better perf

Disk flushes are *extremely* expensive, and ImportantFileWriter's
WriteFileAtomically function does one for each file it writes. That
increases the cost (measured on my Z620 with a spinning platter boot
drive) of creating a set of Windows jumplist icons from ~25 ms to
~250-400 ms.

Yes, disk flushes (plus the deletes and moves, but mostly the disk
flushes) make creating the jumplist icons more than ten times
more expensive. That explains why JumpList::RunUpdateOnFileThread takes
an average of 773 ms on customer machines.

There is no clear reason why a disk flush should be necessary since
database integrity does not seem to be a concern. This will resolve a
long-standing customer complaint.

The change to write all icons came in with CL 15080002 but the reason
does not seem applicable to jumplist icons.

BUG= 40407 

Review URL: https://codereview.chromium.org/1372303002

Cr-Commit-Position: refs/heads/master@{#351692}

[modify] http://crrev.com/dfa983eaa8492758ccf761676820f763220f5c9a/chrome/browser/jumplist_win.cc
[modify] http://crrev.com/dfa983eaa8492758ccf761676820f763220f5c9a/ui/gfx/icon_util.cc
[modify] http://crrev.com/dfa983eaa8492758ccf761676820f763220f5c9a/ui/gfx/icon_util.h

The change referred to in comment #73 (available in M47+, which is canary right now) makes jumplist creation run more than twice as fast, however this is not as much of an improvement as was expected. It might just be that the disk operations are more expensive when not in the cache, as happens when not repeatedly benchmarking this scenario.

If anyone is motivated they can record a Chrome trace at chrome://tracing. Select the "browser" and "toplevel" categories in "Manually select settings". The JumpList update will show up in the Browser process, in Chrome_FileThread, with src_func JumpList::DeferredRunUpdate. In the attached screenshot it is shown in the bottom as having a Wall Duration of 156.929 ms.

If the JumpList update frequently takes longer on your machine then an ETW trace as described here would let me understand what is going on:
https://randomascii.wordpress.com/2015/09/01/xperf-basics-recording-a-trace-the-ultimate-easy-way/
For best results following the extra bonus steps for Chrome and checking the browser and toplevel categories would be helpful.

If you are running M46 or earlier then don't bother recording traces as there is already a fix coming that will improve performance once M47+ get to you.

Jumplist tracing.PNG
46.2 KB View Download
In a quick test I did, the difference is colossal already. "flushes are *extremely* expensive" is really an understatement. Chrome was doing scores of writes when at the same time was essentially disabling the disk cache. Now even with 20 windows (which due to #71 means jumplists are created 20 times)  disk activity isn't  really noticeable.

I have some naive questions such as if there is need for Profile\JumplistIcons(/old) folders when chrome already has all its favicons cached or is there a need(by Win API) to rewrite all the jumplist icons and not just add the new ones but all these are little details relative to how bad Chrome was behaving until now.

Thank you for the feedback. I'm glad that this change has helped.

I think it is possible to only update the icons that have changed, although this is more error prone and is probably not possible until the issue mentioned in #71 is corrected, at which point the benefit will be reduced anyway.

Comment 77 by kerem...@gmail.com, Nov 12 2015

FWIW, 65535 is the number of filenames that can be generated by GetTempFileName function with the same prefix without collision.
Possibly related: CppCon 2015: Niall Douglas “Racing The File System" (https://www.youtube.com/watch?v=uhRWMGBjlO8)

Niall Douglas is the developer of Boost AFIO, and has become a Windows filesystem expert because of it.

From his presentation (linked above, skip to ~8:50), it looks like Chrome's file deletion code is incorrect. I'm going to open a separate bug about that.

...this could be also why  Issue #179576  exists - the deletion operation will "randomly" fail. Douglas suggests (when deleting a directory tree) moving the files one level up, and simultaneously renaming to some cryptographically random name, THEN actually deleting the files, and lastly deleting the directory. I'm going to mention this on that issue.
Project Member

Comment 79 by sheriffbot@chromium.org, Jul 9 2016

Status: Archived (was: Assigned)
This bug has been TE-Verified and unmodified for over 90 days, so archiving it. Please reopen if it's still valid.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -TE-Verified-M46 -Action-NeedsReview -TE-Verified-46.0.2485.0
Status: Assigned (was: Archived)
This bug is still in need of some work.
crrev.com/965503002 added some throttling so we don't write jumplist icons as frequently.
crrev.com/1372303002 avoided doing flushes, so that writing jumplist icons is not as expensive.
However, as mentioned in https://bugs.chromium.org/p/chromium/issues/detail?id=40407#c71 there is still a bug where the entire jumplist icon writing process is repeated once for every top-level Chrome window that is open.

The multiple top-level window bug should be fixed. It is quite possible that other improvements could be made, such as not repeatedly writing the same icons.

Related bugs include crbug.com/583641 and  crbug.com/179576 

Labels: Hotlist-Enterprise
Cc: brucedaw...@chromium.org
Owner: kulshin@chromium.org
Status: Started (was: Assigned)
Status: Fixed (was: Started)
#82 should fix the remaining major problem with jumplist update. Further improvements can be tracked in  issue 648357 

Comment 85 by dan...@d15.biz, Oct 19 2016

I've been encountering very high CPU usage in Chrome due to the JumpListIcons folder containing 65535 files and Chrome continuously iterating over all the files for some reason. Every few days I need to totally delete the directory to keep my computer usable while Chrome is running. Will the changes mentioned in comment #82 fix that? How long until that lands in a Chrome release?
The changes in comment #82 may (if you have multiple top-level Chrome Windows) make the temporary files accumulate slightly more slowly. However it will not fix your issue.

We are aware of your particular problem but are not clear on what causes it. I think that it happens due to directory permission errors, or directory corruption issues. For some reason Chrome becomes unable to delete its temporary files and they then accumulate endlessly. If we understood the problem better then we could fix it easily.

Depending on how technically savvy you are you may be able to give us some valuable information. What would be ideal would be:
1) Install sysinternals process monitor (procmon.exe) from https://technet.microsoft.com/en-us/sysinternals/processmonitor.aspx
2) Configure it to monitor all accesses to the relevant folders (JumpListIcons and JumpListIconsOld, or maybe their parent)
3) Clear out those directories
4) Start recording with procmon
5) Use Chrome for a bit, including opening and closing a few tabs, and leaving it idle long enough for the jumplist tasks to catch up
6) Stop capturing, save the procmon data to a .pml file, and attach to this bug.

It would also be useful to have a rough description of what you did during the recording. Such as - opened three tabs, browsed to these three web sites, closed the tabs in quick succession. Waited ten seconds. Stopped recording.

To set up the filtering in Procmon go to the Filter Menu, Filter..., then add an entry that says "Path", "contains", "JumpList". That's it.

Owner: chengx@chromium.org
Status: Started (was: Fixed)
Some work has been done trying to fix this bug, which can be found in the comment list. However, all of them just make the temporary files accumulate slower, rather than addressing this bug thoroughly. 

I am working on finding the root cause and trying to fix it. I will re-open the bug. Besides, I believe  bug 179576  has the same root cause as this one, just that they are with different Windows versions. So I will merge  bug 179576  into this one.
Cc: ashej...@chromium.org chengx@chromium.org
 Issue 179576  has been merged into this issue.
 Issue 648357  has been merged into this issue.
Project Member

Comment 90 by bugdroid1@chromium.org, Dec 15 2016

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

commit 76561121d20d72e2d54a9071fe30b1db23b128ad
Author: chengx <chengx@chromium.org>
Date: Thu Dec 15 07:53:57 2016

Fix base::CopyDirectory() and JumpListIcons(Old) folders' operation logic

JumpListIcons(Old) folders are getting unexpectively huge for some users.
This is caused by file operation failures.

After analyzing those two folders' operation metrics by landing several changes,
we find that there are a few issues that need to be fixed.

1) base::CopyDirectory() in file_util_win.cc has incorrect logic when target
dir string contains source dir string. Please see detailed comments in the code,
2) When it fails to delete folder |JumpListIconOld|, we should not move
folder |JumpListIcon| to |JumpListIconOld|, otherwise |JumpListIconOld| can
get unexpectedly huge eventually.
3) When it fails to move folder |JumpListIcon| to |JumpListIconOld|, we
should still delete |JumpListIcon| to avoid endless file accumulation in it.

To fix the first issue, IsParent() function is used in file_util_win.cc to
replace the old logic. I have tested it in a previously checked-in CL.
The gathered user data confirms its correctness. It can be found in line
370 of the old version of jumplist.cc in this CL. In that version, I copied
and changed a few file operation functions from //base for debugging and
testing purposes.

We still log one kind of user data for verification and further analysis.

This CL also reverts several changes I made for debugging purpose.

BUG= 40407 , 179576 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2570693002
Cr-Commit-Position: refs/heads/master@{#438778}

[modify] https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad/base/files/file_util_win.cc
[modify] https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/76561121d20d72e2d54a9071fe30b1db23b128ad/tools/metrics/histograms/histograms.xml

Project Member

Comment 91 by bugdroid1@chromium.org, Dec 16 2016

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

commit 5f1289df7c7dabe8cc73704108a1f433d645a62c
Author: chengx <chengx@chromium.org>
Date: Fri Dec 16 03:38:32 2016

A follow-up nit fix for jumplist file operations

After checking in the CL which tries to address the Jumplisticon
performance bug, grt@ pointed out there are a few nits need to be
fixed, including inconsistent code change in different OS files,
unnecessary code and comments.

I copy and paste the comments from grt@ in line with the code in
this CL and reply direclty there.

BUG= 40407 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Review-Url: https://codereview.chromium.org/2577163002
Cr-Commit-Position: refs/heads/master@{#439005}

[modify] https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c/base/files/file_util_posix.cc
[modify] https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c/base/files/file_util_win.cc
[modify] https://crrev.com/5f1289df7c7dabe8cc73704108a1f433d645a62c/chrome/browser/win/jumplist.cc

Components: -UI>OSIntegration Internals>PlatformIntegration
Deprecating UI>OSIntegration in favor of the more generic Internals>PlatformIntegration
 Issue 604130  has been merged into this issue.
Cc: tkonch...@chromium.org
 Issue 621494  has been merged into this issue.
I have checked in a fix to this bug. Though I think this can fix this bug (or part of it), I was not able to verify it because I cannot reproduce the bug locally on my machine. 

This fix is available in the Canary version of Chrome, not the stable version (most of) you are using yet. If you can try the Canary version and see if you still have this issue, that will help us better understand the bug and the current fix. The Canary version of Chrome can be downloaded via https://www.google.com/chrome/browser/canary.html

You will find "JumplistIcons" folder for Canary in a similar path to the one you have trouble with now. Is should be like:
C:\Users\<UserAccount>\AppData\Local\Google\Chrome SxS\User Data\Default\JumpListIcons

We have two Chrome Canary users reported this fix is working for them already as mentioned in  bug 179576 , which sounds promising. However, we definitely want feedback from more users, as there may be other causes (which we don't know yet) to this bug that the current fix does not cover. 

Thank you!
Can confirm that I also have this issue with Chrome and the jumplist folders. jumplistOLD is corrupted, and save dialog box would open at random times.
Cc: nyerramilli@chromium.org
 Issue 326358  has been merged into this issue.
Cc: -hichris...@gmail.com
Cc: jar@chromium.org mgiuca@chromium.org karen@chromium.org
 Issue 278395  has been merged into this issue.
Project Member

Comment 100 by bugdroid1@chromium.org, Mar 3 2017

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

commit eae9a2c34f7315b1aaa17bf265c879681106c90a
Author: chengx <chengx@chromium.org>
Date: Fri Mar 03 21:13:02 2017

Not update jumplist icons when deletion of JumplistIcon folder fails

According to UMA, the deletion of JumplistIcons folder fails for a small
portion of Chrome users. In this case, we should skip updating the jumplist
icons which will bloat the folder. In addition, there is no need to run
base::CreateDirectory() trying to create the same folder.

BUG= 40407 , 179576 

Review-Url: https://codereview.chromium.org/2717813002
Cr-Commit-Position: refs/heads/master@{#454674}

[modify] https://crrev.com/eae9a2c34f7315b1aaa17bf265c879681106c90a/chrome/browser/win/jumplist.cc

Project Member

Comment 101 by bugdroid1@chromium.org, Mar 8 2017

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

commit 051b0af2835a602463b2960894f296f3dcc22680
Author: chengx <chengx@chromium.org>
Date: Wed Mar 08 18:00:30 2017

Skip updating jumplist icons if creation of folder JumplistIcons fails

According to UMA, 0.14% of jumplist IO operations fail to create folder
JumplistIcons after its successful deletion for unknown reasons. This
could impact roughly 0.14% Chrome users. In this case, updating jumplist
icons is meaningless and should be skipped.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2739683003
Cr-Commit-Position: refs/heads/master@{#455489}

[modify] https://crrev.com/051b0af2835a602463b2960894f296f3dcc22680/chrome/browser/win/jumplist.cc

Project Member

Comment 102 by bugdroid1@chromium.org, Mar 13 2017

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

commit 2d05b7ac09a50214f67c7a87c875c9cd6544dce1
Author: chengx <chengx@chromium.org>
Date: Mon Mar 13 22:11:29 2017

Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete

In jumplist related IO operations, one step is to move folder
JumplistIcon to JumplistIconOld using base::Move(). The method tries to
rename the folder and copy-n-delete if renaming fails. For users with
these folders corrupted and bloated already, copy-n-delete is very
expensive and may lead those icon files to be never deleted when e.g.,
copy succeeds yet delete fails.

As folders JumplistIcon and JumplistIconOld are in the same disk volume,
renaming should be working fine and enough for users whose jumplists are
working normally. So this change won't affect those Chrome users at all.
Local tests have been done on my machine and proved it.

This CL also updates the logic when deletion of folder JumplistIcon
fails. Previous logic is that if base::DeleteFile() function call fails,
updating jumplist is skipped. This can be potentially wrong if deletion
of the content in JumplistIcon succeeds but deletion of folder alone
fails. In this case, we should still continue with jumplist update,
which is fixed in this CL. UMA histogram is also updated to record this
change.

UMA metrics will be kept being monitored for this change.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2732313002
Cr-Commit-Position: refs/heads/master@{#456505}

[modify] https://crrev.com/2d05b7ac09a50214f67c7a87c875c9cd6544dce1/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/2d05b7ac09a50214f67c7a87c875c9cd6544dce1/tools/metrics/histograms/histograms.xml

Cc: -chengx@chromium.org
Project Member

Comment 104 by bugdroid1@chromium.org, Mar 24 2017

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

commit 2eefdd30dac56219587f39e35eac276444470401
Author: chengx <chengx@chromium.org>
Date: Fri Mar 24 00:24:07 2017

Remove JumpListIconsOld directory and set upper limit for delete attempts

Current implementation for jumplist involves deleting JumpListIconOld
followed by moving JumpListIcons to JumpListIconsOld. In addition to
that holding icons in JumpListIconsOld has little benefits, this method
is problematic as we assume file's deletion and move are perfect while
they are not. There are certain unexpected situations where these steps
can fail, which has been proved by UMA metrics. This CL removes any
existing file operations related to JumpListIconsOld. A background
task is added to delete JumpListIconsOld for all users.

Also in one jumplist icons update, JumpListIconsOld and JumpListIcons
folders are tried to be deleted, which is a step with heavy IO involved.
Given that the UMA metrics are showing there are still quite a number
of updates taking a very long time, it is skeptical that base::Delete
has issues in some situations, e.g., when antivirus software is scanning
the icon files. It can be the case that base::Delete marks the files
deleted yet they are still there, which causes the files to be read in
every jumplist update.

A method to handle this case is to set an upper limit for the amount
of jumplist icons deleted as well as the amount of attempt failures
per update. This can avoid the jumplist file operations from taking a
very long time. Typically, there are less than 30 jumplist icons in
JumpListIcons(Old) folder, so setting the upper limit to be 100 is a
reasonable choice. This ensures a number of icons can be deleted per
update for users whose jumpilst folders are bloated without holding
the IO thread for an excessive long time.

Customized delete methods for JumpListIcons(Old) directories are created,
which allows limiting the time spent on file deletion and recording all
the possible failure reasons. These methods are put in new files named
jumplist_file_util.{h,cc}. Unit tests for these methods are also created.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2752063002
Cr-Commit-Position: refs/heads/master@{#459297}

[modify] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/BUILD.gn
[modify] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/win/jumplist.h
[add] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/win/jumplist_file_util.cc
[add] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/win/jumplist_file_util.h
[add] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/browser/win/jumplist_file_util_unittest.cc
[modify] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/chrome/test/BUILD.gn
[modify] https://crrev.com/2eefdd30dac56219587f39e35eac276444470401/tools/metrics/histograms/histograms.xml

Project Member

Comment 105 by bugdroid1@chromium.org, Mar 28 2017

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

commit 4e4edb9d083a96781c37b447cf47e4efd7ca8eb7
Author: chengx <chengx@chromium.org>
Date: Tue Mar 28 05:56:43 2017

Delete JumpListIconsOld and update revised links in every update

This CL includes the following changes:

1) In crrev.com/2752063002 landed recently, when JumpListIcons is not
empty or doesn't exist, we are not trying to delete JumpListIconsOld.
This is not optimal, as deleting JumpListIconsOld is now independent of
the folder status of JumpListIcons.

2) When the JumpListIcons folder is not empty after the deletion of its
content, currently we skip creating the new icons as well as updating
the revised links. However, updating the links shouldn't be skipped as
it doesn't involve file IO. This CL fixes it.

3) This CL logs more detailed directory information for jumplist folders
to UMA. In addition to logging if the folder is empty or not, it now
also logs if the folder exists or not.

4) Use base::IsDirectoryEmpty to replace PathIsDirectoryEmpty Windows
API. Thus we can remove the inclusion of Shlwapi.h in a few files.

5) Reduce the limit of files to delete from 100 to 60. One reason is
that setting the limit to more than twice of the normal amount is more
than enough. Another reason is that we are now trying to delete
JumpListIconsOld folder in every jumplist update.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2778513002
Cr-Commit-Position: refs/heads/master@{#460023}

[modify] https://crrev.com/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7/chrome/browser/win/jumplist_file_util.h
[modify] https://crrev.com/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7/chrome/browser/win/jumplist_file_util_unittest.cc
[modify] https://crrev.com/4e4edb9d083a96781c37b447cf47e4efd7ca8eb7/tools/metrics/histograms/histograms.xml

Cc: msrchandra@chromium.org brajkumar@chromium.org chengx@chromium.org
 Issue 401391  has been merged into this issue.
Cc: krajshree@chromium.org kulshin@chromium.org
 Issue 650384  has been merged into this issue.
Cc: beaudoin@chromium.org michaeln@chromium.org tapted@chromium.org erikc...@chromium.org pkasting@chromium.org gab@chromium.org n...@chromium.org ma...@chromium.org
 Issue 439400  has been merged into this issue.
Cc: cpu@chromium.org
 Issue 238267  has been merged into this issue.
Project Member

Comment 110 by bugdroid1@chromium.org, Apr 5 2017

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

commit 971169a30d72887e285e03db2e524a6b3a15d3bb
Author: chengx <chengx@chromium.org>
Date: Wed Apr 05 23:55:55 2017

Migrate jumplist update task from FILE thread to base/task_scheduler

Currently, the jumplist update task is running on FILE thread, which can
hang the FILE thread so that no other tasks can be running on this thread
during that time. The FILE thread has been deprecated and it is preferred
to base/task_scheduler/post_task.h for new classes requiring a background
file I/O task runner.

This CL does this migration for the jumplist update task.

BUG= 40407 , 179576 

Review-Url: https://codereview.chromium.org/2779403002
Cr-Commit-Position: refs/heads/master@{#462280}

[modify] https://crrev.com/971169a30d72887e285e03db2e524a6b3a15d3bb/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/971169a30d72887e285e03db2e524a6b3a15d3bb/chrome/browser/win/jumplist.h
[modify] https://crrev.com/971169a30d72887e285e03db2e524a6b3a15d3bb/chrome/browser/win/jumplist_file_util.cc

Project Member

Comment 111 by bugdroid1@chromium.org, Apr 10 2017

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

commit f1f0e8de131c57105cff16a07d8b9772e0ba13f6
Author: chengx <chengx@chromium.org>
Date: Mon Apr 10 17:00:55 2017

Fix UMA metric "WinJumplist.DirectoryStatusJumpListIconsOld"

The base::IsDirectoryEmpty() method returns true when the directory
doesn't exist or the directory is empty. We need to use
base::DirectoryExists() together with base::IsDirectoryEmpty() to
differentiate these two situations.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2806873002
Cr-Commit-Position: refs/heads/master@{#463297}

[modify] https://crrev.com/f1f0e8de131c57105cff16a07d8b9772e0ba13f6/chrome/browser/win/jumplist_file_util.cc

Project Member

Comment 112 by bugdroid1@chromium.org, Apr 10 2017

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

commit daa4c6f41ab1da23326f2ca39c863a0384587ed6
Author: chengx <chengx@chromium.org>
Date: Mon Apr 10 21:19:15 2017

Separate JumpListIcons delete task and update task

The JumpListIcons delete task deletes the existing icons in JumpListIcons,
while the update task creates the new icons, writes them into JumpListIcons
and notifies the shell. It is better to separate them so we can track
the individual cost. Since a base::SingleThreadTaskRunner is used for
posting these tasks, the tasks are always running in the correct order.

This CL also adds a base::SequencedTaskRunner for JumpListIconsOld
delete tasks as they are not required to run on a single thread.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2807883002
Cr-Commit-Position: refs/heads/master@{#463396}

[modify] https://crrev.com/daa4c6f41ab1da23326f2ca39c863a0384587ed6/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/daa4c6f41ab1da23326f2ca39c863a0384587ed6/chrome/browser/win/jumplist.h
[modify] https://crrev.com/daa4c6f41ab1da23326f2ca39c863a0384587ed6/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/daa4c6f41ab1da23326f2ca39c863a0384587ed6/chrome/browser/win/jumplist_file_util.h

Project Member

Comment 113 by bugdroid1@chromium.org, Apr 11 2017

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

commit a70a095814dfc2530cc653d92962f1bc0b83987d
Author: chengx <chengx@chromium.org>
Date: Tue Apr 11 22:44:06 2017

Experiment with USER_VISIBLE priority for update_jumplisticons_task_runner_

It is preferable to use BACKGROUND priority for jumplist update thread
as it is now. This CL is to see how long it takes for jumplist update if
we assign the thread USER_VISIBLE priority.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2811003002
Cr-Commit-Position: refs/heads/master@{#463812}

[modify] https://crrev.com/a70a095814dfc2530cc653d92962f1bc0b83987d/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/a70a095814dfc2530cc653d92962f1bc0b83987d/chrome/browser/win/jumplist.h
[modify] https://crrev.com/a70a095814dfc2530cc653d92962f1bc0b83987d/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/a70a095814dfc2530cc653d92962f1bc0b83987d/chrome/browser/win/jumplist_file_util.h

Project Member

Comment 115 by bugdroid1@chromium.org, Apr 13 2017

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

commit 6f797ffc358c069eb954a59ea811c12d80208feb
Author: chengx <chengx@chromium.org>
Date: Thu Apr 13 19:42:28 2017

Log UpdateJumpList() and CreateIconFiles() execution time to UMA

The separation of JumpListIcons delete tasks and update tasks as in
crrev.com/2807883002 indicates the update task (i.e., RunUpdateJumpList
method) is now the major contributor of the overall long execution time.
So it's time to investigate the subtasks in RunUpdateJumpList().

The count of the new icon files that will be created per update is also
logged to UMA.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2811393002
Cr-Commit-Position: refs/heads/master@{#464510}

[modify] https://crrev.com/6f797ffc358c069eb954a59ea811c12d80208feb/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/6f797ffc358c069eb954a59ea811c12d80208feb/tools/metrics/histograms/histograms.xml

Project Member

Comment 116 by bugdroid1@chromium.org, Apr 17 2017

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

commit ce5d2adea8adbc9e950371ef08513de8e83d790d
Author: chengx <chengx@chromium.org>
Date: Mon Apr 17 20:19:09 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}

[modify] https://crrev.com/ce5d2adea8adbc9e950371ef08513de8e83d790d/chrome/browser/win/jumplist_updater.cc
[modify] https://crrev.com/ce5d2adea8adbc9e950371ef08513de8e83d790d/tools/metrics/histograms/histograms.xml

Project Member

Comment 117 by bugdroid1@chromium.org, Apr 17 2017

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

commit a27e1c2a48ee40ed2df55effe6d7a15242dd6933
Author: chengx <chengx@chromium.org>
Date: Mon Apr 17 20:31:03 2017

Fix to not create jumplist icon files that aren't used by shell

The current implementation creates more icon files than necessary. In
more details, it first creates icon files for all the retrieved most
visited pages and recently closed pages. The total number of icon files
created is up to 24. However, only at most 10 icons will be used by the
shell. The extra file IO is completely unnecessary.

This CL fixes this issue by only creating the icon files that are used
by shell.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2816113002
Cr-Commit-Position: refs/heads/master@{#465002}

[modify] https://crrev.com/a27e1c2a48ee40ed2df55effe6d7a15242dd6933/chrome/browser/win/jumplist.cc

Project Member

Comment 118 by bugdroid1@chromium.org, Apr 18 2017

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

commit 88ce9f2e84043c5f63cfb7a05cceffef8a581e97
Author: chengx <chengx@chromium.org>
Date: Tue Apr 18 04:11:32 2017

Merge jumplist delete task and update task

This allows consistent comparison of time cost to the previous versions
of jumplist implementation.

Code-wise, it merges the DeleteDirectoryAndLogResults method into the
RunUpdateJumplist method.

This CL also logs the execution time of DeleteDirectoryContentAndLogResults
to UMA.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2827563002
Cr-Commit-Position: refs/heads/master@{#465136}

[modify] https://crrev.com/88ce9f2e84043c5f63cfb7a05cceffef8a581e97/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/88ce9f2e84043c5f63cfb7a05cceffef8a581e97/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/88ce9f2e84043c5f63cfb7a05cceffef8a581e97/tools/metrics/histograms/histograms.xml

Project Member

Comment 119 by bugdroid1@chromium.org, Apr 19 2017

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

commit 45d7b0fd5122143bca4dfc84370e11d7594cd411
Author: chengx <chengx@chromium.org>
Date: Wed Apr 19 19:05:01 2017

Various logical fixes for jumplist

This CL consists of the following three changes:

1) Move DeleteDirectoryContentAndLogResults() so that it runs only if
   jumplist_updater.BeginUpdate() succeeds, otherwise it's pointless.

2) Return early if any AddShellLink call fails. This is suspected to
   cause long execution time for jumplist_updater.

3) Decrease kFileDeleteLimit from 60 to 30. A user without jumplist
   issues should have at most 10 icon files in JumpListIcons folder as
   in crrev.com/2816113002. So changing kFileDeleteLimit to 30 won't
   affect "healthy" users. It'll give a better experience for users
   who have corrupted jumplist folders as the disk IO per jumplist
   update becomes less.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2824103003
Cr-Commit-Position: refs/heads/master@{#465689}

[modify] https://crrev.com/45d7b0fd5122143bca4dfc84370e11d7594cd411/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/45d7b0fd5122143bca4dfc84370e11d7594cd411/chrome/browser/win/jumplist_file_util.h
[modify] https://crrev.com/45d7b0fd5122143bca4dfc84370e11d7594cd411/chrome/browser/win/jumplist_updater.cc

Project Member

Comment 120 by bugdroid1@chromium.org, Apr 25 2017

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

commit 250b02089b81116d27bb3732ada0f8e87de02932
Author: chengx <chengx@chromium.org>
Date: Tue Apr 25 16:22:56 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 a
crazy amount of time, as the following steps (delete old icons, create
new icons, 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}

[modify] https://crrev.com/250b02089b81116d27bb3732ada0f8e87de02932/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/250b02089b81116d27bb3732ada0f8e87de02932/chrome/browser/win/jumplist_updater.cc
[modify] https://crrev.com/250b02089b81116d27bb3732ada0f8e87de02932/tools/metrics/histograms/histograms.xml

Project Member

Comment 121 by bugdroid1@chromium.org, Apr 25 2017

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

commit b60799e04934ed8a06d47beafffd1d28b995915f
Author: chengx <chengx@chromium.org>
Date: Tue Apr 25 20:25:51 2017

Fix the number of JumpList slots assigned to each JumpList category

By default, the maximum number of items to display in JumpList is 10
per Windows. Previously, we considered there'are 10 available JumpList
slots, and we assign these slots to "Most visit" and "Recently closed"
categories accordingly. However, each category title takes one slot.
Therefore, the number of JumpList slots assigned to each category is
incorrect. This caused  bug 714925 , "Recently closed category of
JumpList won't show up until closing three websites instead of one".

To fix this, we need to adjust the number of available JumpList slots
based on the presence of the two JumpList categories. If one of the
categories presents, there'll be one available slot less. If both
categories present, there'll be two available slots less.

Since now the number of available slots is 1 or 2 less as it should
be, we're now deleting and creating 1 or 2 less icons per JumpList
update. This saves some disk IO. Note that the current implementation
deletes 10 icons followed by creating 10 icons per JumpList update.

BUG= 714925 ,  40407 ,  179576 

Review-Url: https://codereview.chromium.org/2844463003
Cr-Commit-Position: refs/heads/master@{#467094}

[modify] https://crrev.com/b60799e04934ed8a06d47beafffd1d28b995915f/chrome/browser/win/jumplist.cc

Project Member

Comment 122 by bugdroid1@chromium.org, Apr 27 2017

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

commit 63cfe1991cbd66f86963ed2e1afecb1c2ed5519e
Author: chengx <chengx@chromium.org>
Date: Thu Apr 27 21:05:09 2017

Update different JumpList categories on demand

The JumpList class is an observer of the TopSites class as well as the
TabRestoreService class. Each time TopSites gets updated or a tab is
removed, the JumpList is updated. Chrome JumpList has two categories,
namely "Most visited" and "Recently closed", whose icon files are
updated altogether in a single JumpList update run. The icon files are
updated in the way that old icons are deleted followed by new icons'
creation.

However, updating icons for both categories together is unnecessary.
When the TopSites class has changes, it affects the "Most visited"
category only; while when the TabRestoreService class has changes, it
affects the "Recently closed" only. In this sense, we should update
each category on demand rather than in every JumpList update even when
there's no change for that category.

Initially, each JumpList update involves 24 icons' deletion and 24
icons' creation. After crrev.com/2816113002 (Fix to not create
jumplist icon files that aren't used by shell) was landed, this number
was reduced from 24*2 to 10*2. This CL further reduced this number to
~4*2 per JumpList update. As each icon file is 28 KB, these two CLs
together have reduced the disk IO from 48*28=1344 KB to 8*28=224 KB by
83% per JumpList update.

This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since only one category is updated for almost all the
time, we'll still have icons for the other category if the OS
notification step fails. That says, we'll still have about half of the
icons instead of nothing, which's clearly better.

This CL changes the JumpList related directory. Since the icon files
for the two categories are dealt with separately, it's more efficient
to put them in separate folders rather than in a single JumpListIcons
folder. This CL introduces two new folders JumpListIconsMostVisited
and JumpListIconsRecentClosed for this purpose. As the JumpListIcons
folder is no longer needed, this CL posts a background task to delete
it.

BUG= 40407 ,  179576 ,  715902 ,  716115 

Review-Url: https://codereview.chromium.org/2836873003
Cr-Commit-Position: refs/heads/master@{#467788}

[modify] https://crrev.com/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/63cfe1991cbd66f86963ed2e1afecb1c2ed5519e/chrome/browser/win/jumplist.h

Project Member

Comment 123 by bugdroid1@chromium.org, May 1 2017

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

commit ced3697c2cfa3e1d5814f8bbf9456aaf4a75543b
Author: chengx <chengx@chromium.org>
Date: Mon May 01 18:21:39 2017

Delete JumpList temp file when failing to write the icon's content to it

Currently, an empty temp file is created before a JumpList icon's
content is written to it. However, the temp file is not deleted right
away if the content writing fails, which explains why there're empty
icon temp files in the jumplist folder sometimes. This indicates we're
leaking temp files and may cause performance issues.

This CL fixes this issue by deleting the empty temp files right away
when necessary.

BUG= 40407 ,  179576 ,  715902 ,  716601 

Review-Url: https://codereview.chromium.org/2852763003
Cr-Commit-Position: refs/heads/master@{#468357}

[modify] https://crrev.com/ced3697c2cfa3e1d5814f8bbf9456aaf4a75543b/chrome/browser/win/jumplist.cc

Project Member

Comment 124 by bugdroid1@chromium.org, May 1 2017

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

commit 4f731454df3bdd7076668f4c906dc1eaab38e8ad
Author: chengx <chengx@chromium.org>
Date: Mon May 01 18:43:18 2017

Re-enable UMA metric WinJumplist.CreateIconFilesCount

This UMA metric tracks the number of icons requested to create per
JumpList update. The actual number of icons created may be less if it
fails to create certain icons due to random reasons.

Since I've landed some CLs reducing this number and will try to
further reduce it, I think this metric should be re-enabled to track
the progress.

BUG= 40407 ,  179576 ,  715902 

Review-Url: https://codereview.chromium.org/2848763002
Cr-Commit-Position: refs/heads/master@{#468367}

[modify] https://crrev.com/4f731454df3bdd7076668f4c906dc1eaab38e8ad/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/4f731454df3bdd7076668f4c906dc1eaab38e8ad/tools/metrics/histograms/histograms.xml

Project Member

Comment 125 by bugdroid1@chromium.org, May 1 2017

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

commit 4fffeff4e2367820ccddaa23a9800aa97d887d51
Author: chengx <chengx@chromium.org>
Date: Mon May 01 21:43:16 2017

Retire some metrics and update file util methods for JumpList

This CL retires some file deletion related metrics for JumpList since
they're no longer needed.

Accordingly, the JumpList file deletion related methods are updated to
return nothing instead of the detailed delete status which is used for
the retiring metrics. This makes the ENUMs for the delete status no
long needed so they're removed. These methods are also refactored a
little bit to make them cleaner.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2836363003
Cr-Commit-Position: refs/heads/master@{#468425}

[modify] https://crrev.com/4fffeff4e2367820ccddaa23a9800aa97d887d51/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/4fffeff4e2367820ccddaa23a9800aa97d887d51/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/4fffeff4e2367820ccddaa23a9800aa97d887d51/chrome/browser/win/jumplist_file_util.h
[modify] https://crrev.com/4fffeff4e2367820ccddaa23a9800aa97d887d51/chrome/browser/win/jumplist_file_util_unittest.cc
[modify] https://crrev.com/4fffeff4e2367820ccddaa23a9800aa97d887d51/tools/metrics/histograms/histograms.xml

Project Member

Comment 126 by bugdroid1@chromium.org, May 2 2017

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

commit 07cfbaf67fc15e9bab6d586db6b109685fcc4944
Author: chengx <chengx@chromium.org>
Date: Tue May 02 15:11:22 2017

Update a few JumpList comments

This CL removes some obsolete comments. For example, the class
RecentlyClosedTabsHandler is gone, so the related comments should be
deleted as well.

Other comment updates are according to Google C++ style guide.
https://google.github.io/styleguide/cppguide.html#Comments

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2849113003
Cr-Commit-Position: refs/heads/master@{#468637}

[modify] https://crrev.com/07cfbaf67fc15e9bab6d586db6b109685fcc4944/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/07cfbaf67fc15e9bab6d586db6b109685fcc4944/chrome/browser/win/jumplist.h

Project Member

Comment 127 by bugdroid1@chromium.org, May 4 2017

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

commit a16438df21e2fb1b99b8b3f6b80180407c78d3cc
Author: chengx <chengx@chromium.org>
Date: Thu May 04 17:08:21 2017

Log runtime of StartLoadingFavicon and OnFaviconDataAvailable for
JumpList

Currently there're lots of redundant of favicon loading calls in
JumpList update. The total time cost of one favicon loading process
consists of the runtime of one StartLoadingFavicon call, one
asynchronous GetFaviconImageForPageURL call and one
OnFaviconDataAvailable call.

UMA currently knows the total amount of GetFaviconImageForPageURL
function calls and their runtime distribution. However, UMA doesn't
know its exact contribution from the JumpList update. It would be nice
to log some information from the JumpList class so that we know how
much we can possibly improve.

BUG= 40407 ,  179576 ,  717236 

Review-Url: https://codereview.chromium.org/2860573005
Cr-Commit-Position: refs/heads/master@{#469365}

[modify] https://crrev.com/a16438df21e2fb1b99b8b3f6b80180407c78d3cc/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/a16438df21e2fb1b99b8b3f6b80180407c78d3cc/tools/metrics/histograms/histograms.xml

Project Member

Comment 128 by bugdroid1@chromium.org, May 8 2017

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

commit 9c0c398442ed5dc4d3c52f735b57cb84a22c977d
Author: chengx <chengx@chromium.org>
Date: Mon May 08 19:57:37 2017

Filter redundant JumpList favicons' fetching and related

In the current JumpList implementation, each time when the TopSites
service or the TabRestore service has updates, the JumpList being an
observer of them will start an update.

This update consists of the following steps:
1) Retrieve updated URLs, create ShellLinkItem and fetch the favicons
   for these URLs;
2) Schedule a RunUpdateJumpList run to update icons in jumplisticon
   folders and notify the shell.

Currently, there's a delay of 3500 ms before each RunUpdateJumpList
call so that other RunUpdateJumpList calls requested in this period
will be collapsed into one which can prevent update storms. This says
that we are coalescing step 2 of jumplist updates which are close to
each other in time.

However, we're not coalescing step 1 of those jumplist updates, which
makes some of them completely wasted. Therefore, we should apply this
"delay and coalesce" strategy to step 1 as well.

Moreover, in JumpList::OnMostVisitedURLsAvailable(), we're processing
more URLs (typically 24) than needed (typically between 5 to 9). The
work of creating ShellLinkItems and fetching favicons for these URLs
is wasted.

Note that all these tasks are running on UI thread. Therefore, it's
very important to optimize them which otherwise can possibly hang
Chrome.

BUG= 40407 ,  179576 ,  717236 

Review-Url: https://codereview.chromium.org/2859693002
Cr-Commit-Position: refs/heads/master@{#470091}

[modify] https://crrev.com/9c0c398442ed5dc4d3c52f735b57cb84a22c977d/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/9c0c398442ed5dc4d3c52f735b57cb84a22c977d/chrome/browser/win/jumplist.h

Project Member

Comment 129 by bugdroid1@chromium.org, May 10 2017

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

commit e69eb0e891909eacc2ad5b22787df30517428ea4
Author: chengx <chengx@chromium.org>
Date: Wed May 10 04:44:33 2017

Change 4 funcs to JumpList member funcs, retire wstring for jumplist*

This CL seems to have a lot of code changes, but in fact it does
nothing but to change 4 anonymous functions to JumpList class member
functions. Originally these 4 functions were put in an anonymous
namespace, and they are now moved to the end of jumplist.cc. Their
declarations are added in jumplist.h. The body of these 4 functions
have no changes at all. However, one PostTask function call is updated
accordingly due to this change.

Making these 4 functions be member functions of JumpList class allows
them to get access the member variables (which I am going to add) of
the JumpList class. This lays out the foundation for a number of
JumpList improvements I am going to do in parallel. Otherwise, there
will be a lot of duplicated code changes and heavy merge conflicts
ahead. Moreover, this also makes it easier to review the coming CLs on
top of this.

This CL also changes all std::wstring to base::string16 in jumplist.*
and jumplist_updater.* to get rid of presubmit warnings.

BUG= 40407 ,  179576 

Review-Url: https://codereview.chromium.org/2870853002
Cr-Commit-Position: refs/heads/master@{#470455}

[modify] https://crrev.com/e69eb0e891909eacc2ad5b22787df30517428ea4/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/e69eb0e891909eacc2ad5b22787df30517428ea4/chrome/browser/win/jumplist.h
[modify] https://crrev.com/e69eb0e891909eacc2ad5b22787df30517428ea4/chrome/browser/win/jumplist_updater.cc
[modify] https://crrev.com/e69eb0e891909eacc2ad5b22787df30517428ea4/chrome/browser/win/jumplist_updater.h

Project Member

Comment 130 by bugdroid1@chromium.org, May 12 2017

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

commit 143d0a7278a1f2b6ff9173ec78e6755b7100704c
Author: chengx <chengx@chromium.org>
Date: Fri May 12 20:27:09 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}

[modify] https://crrev.com/143d0a7278a1f2b6ff9173ec78e6755b7100704c/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/143d0a7278a1f2b6ff9173ec78e6755b7100704c/chrome/browser/win/jumplist.h

Sorry for my chatty disposition, but I  think you can also take into account when chrome is 'out of focus'/ 'no  chrome windows active' to do some of the jumplist work. If taskbar isn't in focus, no one is using  any jumplists.

Project Member

Comment 132 by bugdroid1@chromium.org, May 19 2017

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

commit 8127957d51be9220f6d5f64c637cd3a9d5566ea8
Author: chengx <chengx@chromium.org>
Date: Fri May 19 04:05:15 2017

Cache JumpList icons to avoid unnecessary creation and deletion

Previously, when a JumpList category ("Mostly visited" or "Recently
closed") is updated, new icons are created and written into the
corresponding folder after all old icons in the same folder are
deleted. However, there's usually a (big) overlap between the new and
old icons. This means that we're deleting a lot of icons and then
creating the same ones in each update, which is very inefficient.

For example, JumpList can show at most 3 items for "Recently closed"
category. Therefore, there are usually 3 icons existed for this
JumpList category. When a Chrome tab gets closed, the icons for this
category is updated. Previously, we delete all 3 existing icons and
then create 3 new ones. However, the most efficient way is to create
one icon (if it doesn't exist) only for this closed tab and delete the
least recent one. If this icon exists already, we don't need to delete
and create any icon files at all.

To fix this, we introduce two maps to cache the URL-IconFilePath
information of the two JumpList categories. With the help of these
maps, we are able to avoid deleting and creating the same icons.

The JumpList icon folders will be cleaned entirely only in the
following situations:
1) The "Most visited" category updates for the first time after Chrome
   is launched. This actually happens right after Chrome is launched.
2) The "Recently closed" category updates for the first time after
   Chrome is launched. This happens when a first Chrome tab is closed.
3) The number of icons in the jumplist icon folders has exceeded the
   limit, which is currently set to 12 for "most visited" category and
   6 for "recently closed" category, respectively. This is very
   important as it prevents the jumplist folders from getting bloated
   endlessly.

Note that some sites change the favicon to indicate status. Since
caching is done based on the URL rather than the image itself, the
jumplist won't pick up these stats updates. Given the fact that this
cache mechanism improves the performance nicely, this is an okay
compromise.

[Impact on UI side]
This CL also relieves the following issue. Currently, notifying the OS
about the JumpList update takes place after the old icon files are
deleted. This order is critical as it can avoid the JumpList folder
from getting accumulated endlessly. On the other hand, if the OS
notification step fails which does happen sometimes according to UMA
data, the old JumpList will still be used. However, since the old
icons have been deleted, there'll be nothing but the background color
showing up where the icons should show up. This doesn't look good.
With this CL, since we're now only deleting the retired icons rather
than all the old icons, we'll still have most of the old icons if the
OS notification step fails, which makes the JumpList look much better
in this situation.

BUG= 40407 ,  179576 ,  715902 ,  716115 

Review-Url: https://codereview.chromium.org/2859193005
Cr-Commit-Position: refs/heads/master@{#473078}

[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist.h
[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util.cc
[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util.h
[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_file_util_unittest.cc
[modify] https://crrev.com/8127957d51be9220f6d5f64c637cd3a9d5566ea8/chrome/browser/win/jumplist_updater.h

Re comment 131:

Thanks for your suggestion. The fact is, when chrome is 'out of focus' or 'no chrome windows are active', jumplist won't update at all in the current implementation. 
Project Member

Comment 134 by bugdroid1@chromium.org, May 31 2017

Issue 352968 has been merged into this issue.
Cc: rtenneti@chromium.org caitkp@chromium.org
 Issue 117200  has been merged into this issue.
Project Member

Comment 137 by bugdroid1@chromium.org, Jun 13 2017

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

commit 4a5397999a0f493c47632e1a8bd93686de54adbf
Author: chengx <chengx@chromium.org>
Date: Tue Jun 13 19:24:57 2017

Fix stability and data racing issues, coalesce more updates for JumpList

This CL contains the following five main changes plus lots of updates
on variable/method names and comments.

1. Fix the JumpList data-racing issue by separating different thread
tasks.

There are two threads involved in updating the JumpList. The UI thread
lives the module to collect JumpList data for update, and a non-UI
thread lives the RunUpdateJumpList module which updates the JumpList
and notifies the shell. Previously, the two modules were interleaved
to some extent, which required the JumpList data to be shared between
the two threads. This caused the data racing issue and was solved by
introducing a lock. The interleaving of these two modules complicates
the code and makes it very difficult to add unit tests. This CL
separates these two modules. In more details, the JumpList data is now
only accessible to the UI thread. Local copies of the data are made
and sent to the non-UI thread to run the update task. Once that update
task finishes on the non-UI thread, the update results are transferred
to the UI thread via PostTaskAndReply. The UI thread then decides how
to update the JumpList data according to the update results.

2. Fix the JumpList stability issue by making the RunUpdateJumpList
API static so it outlives the JumpList instance.

When a JumpList::RunUpdateJumpList task is posted on a non-UI thread
and about to run, the JumpList instance may have already been
destructed. This caused stability issues as the RunUpdateJumpList API
was non-static. This CL fixes it by making this API and other APIs it
calls static.

3. Coalesce JumpList updates from two services by keeping only one
timer.

Previously, there were two timers to coalesce updates from the
TopSites service and the TabRestore service, respectively. When
notifications from both services came, even at the same time, multiple
RunUpdateJumpList tasks were posted where they should be coalesced
into one. This caused a waste of many shell notification runs. Note
that there was no wasted work on icon operations as different JumpList
categories were processed on demand. This CL trims out this waste by
keeping only one timer to coalesce updates triggered by different
services.

4. Cancel fetching most-visited URLs when Jumplist is destroyed.

This CL makes it possible to cancel the tasks of fetching most-visited
URLs when JumpList is destroyed, which otherwise is wasted. This also
avoids unnecessary favicon loading triggered upon the completion of
the URL fetch.

5. Make JumpList not refcounted.

JumpList is owned by BrowserView. It is RefCounted but there doesn't
appear to be a reason for that. This CL changes JumpList to be not
refcounted by making it a regular keyed service, rather than a
refcounted keyed service as it was.

BUG= 40407 ,  179576 ,  498987 ,  717236 ,  720028 ,  732586 

Review-Url: https://codereview.chromium.org/2931573003
Cr-Commit-Position: refs/heads/master@{#479095}

[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist.h
[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist_factory.cc
[modify] https://crrev.com/4a5397999a0f493c47632e1a8bd93686de54adbf/chrome/browser/win/jumplist_factory.h

Project Member

Comment 138 by bugdroid1@chromium.org, Jun 14 2017

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

commit 184a0e32ca40eac7c5f046aa840b31845e06afc0
Author: chengx <chengx@chromium.org>
Date: Wed Jun 14 17:28:17 2017

Revert of Fix stability and data racing issues, coalesce more updates for JumpList (patchset #6 id:420001 of https://codereview.chromium.org/2931573003/ )

Reason for revert:
Caused crash as recorded in crbug/733319 and crbug/733098

Original issue's description:
> Fix stability and data racing issues, coalesce more updates for JumpList
>
> This CL contains the following five main changes plus lots of updates
> on variable/method names and comments.
>
> 1. Fix the JumpList data-racing issue by separating different thread
> tasks.
>
> There are two threads involved in updating the JumpList. The UI thread
> lives the module to collect JumpList data for update, and a non-UI
> thread lives the RunUpdateJumpList module which updates the JumpList
> and notifies the shell. Previously, the two modules were interleaved
> to some extent, which required the JumpList data to be shared between
> the two threads. This caused the data racing issue and was solved by
> introducing a lock. The interleaving of these two modules complicates
> the code and makes it very difficult to add unit tests. This CL
> separates these two modules. In more details, the JumpList data is now
> only accessible to the UI thread. Local copies of the data are made
> and sent to the non-UI thread to run the update task. Once that update
> task finishes on the non-UI thread, the update results are transferred
> to the UI thread via PostTaskAndReply. The UI thread then decides how
> to update the JumpList data according to the update results.
>
> 2. Fix the JumpList stability issue by making the RunUpdateJumpList
> API static so it outlives the JumpList instance.
>
> When a JumpList::RunUpdateJumpList task is posted on a non-UI thread
> and about to run, the JumpList instance may have already been
> destructed. This caused stability issues as the RunUpdateJumpList API
> was non-static. This CL fixes it by making this API and other APIs it
> calls static.
>
> 3. Coalesce JumpList updates from two services by keeping only one
> timer.
>
> Previously, there were two timers to coalesce updates from the
> TopSites service and the TabRestore service, respectively. When
> notifications from both services came, even at the same time, multiple
> RunUpdateJumpList tasks were posted where they should be coalesced
> into one. This caused a waste of many shell notification runs. Note
> that there was no wasted work on icon operations as different JumpList
> categories were processed on demand. This CL trims out this waste by
> keeping only one timer to coalesce updates triggered by different
> services.
>
> 4. Cancel fetching most-visited URLs when Jumplist is destroyed.
>
> This CL makes it possible to cancel the tasks of fetching most-visited
> URLs when JumpList is destroyed, which otherwise is wasted. This also
> avoids unnecessary favicon loading triggered upon the completion of
> the URL fetch.
>
> 5. Make JumpList not refcounted.
>
> JumpList is owned by BrowserView. It is RefCounted but there doesn't
> appear to be a reason for that. This CL changes JumpList to be not
> refcounted by making it a regular keyed service, rather than a
> refcounted keyed service as it was.
>
> BUG= 40407 ,  179576 ,  498987 ,  717236 ,  720028 ,  732586 
>
> Review-Url: https://codereview.chromium.org/2931573003
> Cr-Commit-Position: refs/heads/master@{#479095}
> Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd93686de54adbf

TBR=grt@chromium.org,pkasting@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 40407 ,  179576 ,  498987 ,  717236 ,  720028 ,  732586 

Review-Url: https://codereview.chromium.org/2937993002
Cr-Commit-Position: refs/heads/master@{#479434}

[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist.h
[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist_factory.cc
[modify] https://crrev.com/184a0e32ca40eac7c5f046aa840b31845e06afc0/chrome/browser/win/jumplist_factory.h

Project Member

Comment 139 by bugdroid1@chromium.org, Jun 15 2017

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

commit f1c120eb244a567ec94bec0e8f43e076963bf550
Author: chengx <chengx@chromium.org>
Date: Thu Jun 15 21:36:36 2017

Fix stability and data racing issues, coalesce more updates for JumpList

This CL contains the following five main changes plus lots of updates
on variable/method names and comments.

1. Fix the JumpList data-racing issue by separating different thread
tasks.

There are two threads involved in updating the JumpList. The UI thread
lives the module to collect JumpList data for update, and a non-UI
thread lives the RunUpdateJumpList module which updates the JumpList
and notifies the shell. Previously, the two modules were interleaved
to some extent, which required the JumpList data to be shared between
the two threads. This caused the data racing issue and was solved by
introducing a lock. The interleaving of these two modules complicates
the code and makes it very difficult to add unit tests. This CL
separates these two modules. In more details, the JumpList data is now
only accessible to the UI thread. Local copies of the data are made
and sent to the non-UI thread to run the update task. Once that update
task finishes on the non-UI thread, the update results are transferred
to the UI thread via PostTaskAndReply. The UI thread then decides how
to update the JumpList data according to the update results.

2. Fix the JumpList stability issue by making the RunUpdateJumpList
API static so it outlives the JumpList instance.

When a JumpList::RunUpdateJumpList task is posted on a non-UI thread
and about to run, the JumpList instance may have already been
destructed. This caused stability issues as the RunUpdateJumpList API
was non-static. This CL fixes it by making this API and other APIs it
calls static.

3. Coalesce JumpList updates from two services by keeping only one
timer.

Previously, there were two timers to coalesce updates from the
TopSites service and the TabRestore service, respectively. When
notifications from both services came, even at the same time, multiple
RunUpdateJumpList tasks were posted where they should be coalesced
into one. This caused a waste of many shell notification runs. Note
that there was no wasted work on icon operations as different JumpList
categories were processed on demand. This CL trims out this waste by
keeping only one timer to coalesce updates triggered by different
services.

4. Cancel fetching most-visited URLs when Jumplist is destroyed.

This CL makes it possible to cancel the tasks of fetching most-visited
URLs when JumpList is destroyed, which otherwise is wasted. This also
avoids unnecessary favicon loading triggered upon the completion of
the URL fetch.

5. Make JumpList not refcounted.

JumpList is owned by BrowserView. It is RefCounted but there doesn't
appear to be a reason for that. This CL changes JumpList to be not
refcounted by making it a regular keyed service, rather than a
refcounted keyed service as it was.

BUG= 40407 ,  179576 ,  498987 ,  717236 ,  720028 ,  732586 

Review-Url: https://codereview.chromium.org/2931573003
Cr-Original-Commit-Position: refs/heads/master@{#479095}
Committed: https://chromium.googlesource.com/chromium/src/+/4a5397999a0f493c47632e1a8bd93686de54adbf
Review-Url: https://codereview.chromium.org/2931573003
Cr-Commit-Position: refs/heads/master@{#479848}

[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist.h
[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist_factory.cc
[modify] https://crrev.com/f1c120eb244a567ec94bec0e8f43e076963bf550/chrome/browser/win/jumplist_factory.h

Project Member

Comment 140 by bugdroid1@chromium.org, Jun 16 2017

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

commit 0cf2ecfa1dfa8ab1626c75d1d27bf01be3e7aa10
Author: chengx <chengx@chromium.org>
Date: Fri Jun 16 19:27:45 2017

Use JumpList icon caches to remove redundant favicon loading

This CL applies the icon caches to save unnecessary favicon loading from
the FaviconService. The icon caches have already been used to trim out
the redundant icon creation and deletion on disk as in crrev/2859193005.
These two changes make the best of the icon caches.

BUG= 40407 ,  717236 

Review-Url: https://codereview.chromium.org/2936053002
Cr-Commit-Position: refs/heads/master@{#480122}

[modify] https://crrev.com/0cf2ecfa1dfa8ab1626c75d1d27bf01be3e7aa10/chrome/browser/win/jumplist.cc

Project Member

Comment 141 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 142 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

Status: Fixed (was: Started)
This bug is fixed!

Starting from M60-Stable which is being pushed out now, the JumpList shouldn't cause things like Chrome hanging for a long time, persistent massive disk IO, high CPU usage and Chrome crashes anymore. The jank rate (>=2.5 s) has reduced by 95% compared to M56-Stable.

There are more improvements in M61-Stable and M62-Stable which make the JumpList update even more efficiently.
Showing comments 44 - 143 of 143 Older

Sign in to add a comment