New issue
Advanced search Search tips

Issue 715902 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Remove redundant JumpList icons' deletion and creation

Project Member Reported by chengx@chromium.org, Apr 27 2017

Issue description

The current JumpList implementation is very inefficient regards to the JumpList icon's operation. There're a lot of redundant icons' deletion and creation per JumpList update. In more details, there're a lot of icons that remain the same during one JumpList update, and therefore should be untouched. However, the current implementation deletes them and then create exactly the same ones again. This leads to a large waste of OS resources and power. It's also harmful to the hard drive. This should be avoided.
 

Comment 1 by chengx@chromium.org, Apr 27 2017

Owner: chengx@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by chengx@chromium.org, Apr 27 2017

I didn't file this bug when I landed the CL crrev.com/2816113002. Since landing this CL is definitely an important step towards fixing this bug, I am manually pasting its description here.

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.

See crrev.com/2816113002 for the details.

Comment 3 by chengx@chromium.org, Apr 27 2017

Cc: gab@chromium.org grt@chromium.org brucedaw...@chromium.org
Components: Internals>PlatformIntegration
Project Member

Comment 4 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 5 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 6 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

Summary: Remove redundant JumpList icons' deletion and creation (was: Remove unnecessary JumpList icons' fetching, deletion and creation)
Description: Show this description
Project Member

Comment 9 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

Project Member

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

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

commit e1cdc972614971510f7de2672de8f782fd07385e
Author: chengx <chengx@chromium.org>
Date: Tue May 23 21:44:17 2017

Log the number of new icons created per jumplist update

This CL helps to validate the improvements led by the ongoing efforts in
removing redundant icon creation.

BUG= 715902 

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

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

Project Member

Comment 11 by bugdroid1@chromium.org, May 29 2017

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

commit 04f0f9434101306e5fe99f0ab76411dc1a75b31d
Author: chengx <chengx@chromium.org>
Date: Mon May 29 08:22:02 2017

Change hard-coded constants to predefined constant variables in JumpList

This CL changes hard-coded constants to predefined constant variables
which were introduced in a previous CL.

BUG= 715902 

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

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

Project Member

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

Status: Fixed (was: Assigned)
Verified in Canary:

Before the fix (Apr 18, Wed.), the daily number of icons created (and deleted later on) is ~40,144,499. 
After the fix (Jul 5, Wed.), the daily number of icons created is ~2,192,595.

The daily workload has dropped by 94.5%.

Project Member

Comment 15 by bugdroid1@chromium.org, Jul 7 2017

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

commit 70e42d3a78a2a4272c42df316b65d0dd35aadc74
Author: Xi Cheng <chengx@chromium.org>
Date: Fri Jul 07 02:46:27 2017

Retire icon creation related UMA metrics

This CL retires WinJumplist.CreateIconFilesCount and
WinJumplist.CreateIconFilesDuration as crbug/715902 is fixed.

Bug:  715902 
Change-Id: I9781790f0ee1d24d31c4c8fe74a2708e07b8ef56
Reviewed-on: https://chromium-review.googlesource.com/562364
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Commit-Queue: Xi Cheng <chengx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#484809}
[modify] https://crrev.com/70e42d3a78a2a4272c42df316b65d0dd35aadc74/chrome/browser/win/jumplist.cc
[modify] https://crrev.com/70e42d3a78a2a4272c42df316b65d0dd35aadc74/tools/metrics/histograms/histograms.xml

Sign in to add a comment