Filter redundant URL retrieval, ShellLinkItem creation and favicon loading in JumpList update |
||||
Issue descriptionIn 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 includes 1) retrieve updated URLs, create ShellLinkItem and load the favicons for these URLs; 2) schedule a RunUpdateJumpList call to update icons in the jumplisticon* folders and notify the shell. Currently, there's a delay of 3500 ms before each RunUpdateJumpList call so that any 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 very close jumplist updates. However, we're not coalescing step 1 of these very close 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 OnMostVisitedURLsAvailable(), we're processing more URLs (typically 24) (ShellLinkItem creation and favicon loading) than needed (between 5 to 9). This is also a waste of resources. Btw, all these tasks are taking place on UI thread. Therefore, it's important to optimize them.
,
May 2 2017
,
May 2 2017
,
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
,
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
,
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 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
,
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
,
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
,
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
,
Jun 20 2017
,
Jun 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fad0f2a4cb07296a0293ccadb9be5e980a517e90 commit fad0f2a4cb07296a0293ccadb9be5e980a517e90 Author: chengx <chengx@chromium.org> Date: Thu Jun 29 20:20:34 2017 Retire JumpList favicon loading related UMA histograms This CL retires WinJumplist.OnFaviconDataAvailableDuration and WinJumplist.StartLoadingFaviconDuration as crbug/717236 is fixed and verified. BUG= 717236 Review-Url: https://codereview.chromium.org/2956553002 Cr-Commit-Position: refs/heads/master@{#483467} [modify] https://crrev.com/fad0f2a4cb07296a0293ccadb9be5e980a517e90/chrome/browser/win/jumplist.cc [modify] https://crrev.com/fad0f2a4cb07296a0293ccadb9be5e980a517e90/tools/metrics/histograms/histograms.xml
,
Jul 1 2017
Before the fix, the number of favicons loaded (May 8, Canary) is ~24,051,472. After the fix, the number of favicons loaded (June 30, Canary) is ~1,529,763. The daily workload has dropped by 94%.
,
Jul 5 2017
Sweet! |
||||
►
Sign in to add a comment |
||||
Comment 1 by chengx@chromium.org
, May 2 2017