Remove redundant JumpList icons' deletion and creation |
|||||
Issue descriptionThe 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.
,
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.
,
Apr 27 2017
,
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
,
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
,
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
,
May 1 2017
,
May 1 2017
,
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
,
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
,
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
,
May 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/61e622b8b398999d4884231999375245c07dad01 commit 61e622b8b398999d4884231999375245c07dad01 Author: chengx <chengx@chromium.org> Date: Wed May 31 20:59:27 2017 Cancel the JumpList update if top 5 most visited URLs are unchanged Previously, if TopSites service detects any changes in the top 24 most visited URLs, a JumpList update is scheduled. However, since only the top 5 URLs are displayed in JumpList, updates caused by the changes outside of the top 5 URLs are wasted. This CL trims out that waste. BUG= 40407 , 179576 , 715902 , 717236 , 498987 Review-Url: https://codereview.chromium.org/2896233003 Cr-Commit-Position: refs/heads/master@{#476017} [modify] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/BUILD.gn [modify] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/win/jumplist.cc [modify] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/win/jumplist_file_util.h [add] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/win/jumplist_update_util.cc [add] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/win/jumplist_update_util.h [add] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/browser/win/jumplist_update_util_unittest.cc [modify] https://crrev.com/61e622b8b398999d4884231999375245c07dad01/chrome/test/BUILD.gn
,
Jun 20 2017
,
Jul 6 2017
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%.
,
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 |
|||||
Comment 1 by chengx@chromium.org
, Apr 27 2017Status: Assigned (was: Untriaged)