Investigate and remove redundant HistoryBackend::QueryMostVisitedURLs runs |
||||
Issue descriptionThe TopSites service is using a combination of observer pattern and polling to keep the top site list up-to-update. This generates a huge number of HistoryBackend::QueryMostVisitedURLs runs, which gives a non-trivial cost. It's worthwhile to investigate whether polling is needed. If not, we should remove it.
,
Sep 21 2017
Thanks for the reminder, Scott. I think I've found what we need to add to the observer pattern so that polling can be safely removed. I'll update the doc (go/improve-topsites) shortly. IMHO, polling should be avoided in general. It not only generates lots of useless function calls, but also makes the top site NOT up-to-date for a big chunk of time.
,
Sep 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/036d436a9eb55d9073580af842b4cf27142eecef commit 036d436a9eb55d9073580af842b4cf27142eecef Author: Xi Cheng <chengx@chromium.org> Date: Thu Sep 21 04:43:34 2017 Log the runtime for HistoryBackend::QueryMostVisitedURLs Chrome is executing lots of HistoryBackend::QueryMostVisitedURLs runs. This CL logs the runtime of this function to UMA, which also provides the number of reporting users. Bug: 767247 Change-Id: Id05ea596a2f62371fa41d6243ca26deaded358db Reviewed-on: https://chromium-review.googlesource.com/675965 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#503354} [modify] https://crrev.com/036d436a9eb55d9073580af842b4cf27142eecef/components/history/core/browser/history_backend.cc [modify] https://crrev.com/036d436a9eb55d9073580af842b4cf27142eecef/tools/metrics/histograms/histograms.xml
,
Sep 22 2017
1. I left my chromium running overnight, and found 10+ more HistoryBackend::QueryMostVisitedURLs calls the next day. So polling is confirmed to exist in TopSites. 2. TopSites does observer navigation events via TopSitesImpl::OnNavigationCommitted in the current implementation. It observes ALL navigation events, but only triggers self refreshing in some situations. Since many of the navigation events are irrelevant though, the throttling strategy is used as mentioned in this API. 3. Browsing history (chrome://history/) only changes when an user navigates to a new page or re-navigates to an existing navigation entry (e.g. history navigation). We should make TopSites observe those relevant navigation events. The polling strategy should be removed, and TopSites should not "observe" InstantService via InstantService::OnNewTabPageOpened anymore.
,
Oct 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/077c51f05cbc3dab4b0c07558012cdb132535204 commit 077c51f05cbc3dab4b0c07558012cdb132535204 Author: Xi Cheng <chengx@chromium.org> Date: Tue Oct 03 16:56:20 2017 Make TopSites observe correct navigation events, remove its infinite spin in refreshing In the old design, TopSites uses a combination of observer pattern and polling to trigger self-refreshing. TopSites "observes" Instant service and refreshes itself when there is a new tab page opened. TopSites also observes all navigation events and throttles them as many of them are irrelevant. Moreover, TopSites periodically polls the browsing history to keep itself update-to-date. This is especially problematic as it periodically wakes up Chrome when it should be idle. This generates lots of redundant QueryMostVisitedURLs runs, which gives a non-trivial cost. TopSites should be refreshed only when the browsing history is changed, i.e., when an user navigates to a new page or re-navigates to an existing page (e.g. history navigation) in the frame browser frame. So we should make TopSites only observe those navigation events. By doing this, the polling strategy can be safely removed. Since TopSites doesn't observe irrelevant navigation events now, the ad-hoc inconsistent delay for throttling purpose can be replaced with a consistent delay. This CL implements these ideas. Bug: 767247 Change-Id: I618a473762a63333cb04322c694bce31aa44e598 Reviewed-on: https://chromium-review.googlesource.com/678115 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Commit-Queue: Xi Cheng <chengx@chromium.org> Cr-Commit-Position: refs/heads/master@{#506072} [modify] https://crrev.com/077c51f05cbc3dab4b0c07558012cdb132535204/components/history/content/browser/web_contents_top_sites_observer.cc [modify] https://crrev.com/077c51f05cbc3dab4b0c07558012cdb132535204/components/history/core/browser/top_sites_impl.cc [modify] https://crrev.com/077c51f05cbc3dab4b0c07558012cdb132535204/components/history/core/browser/top_sites_impl.h [modify] https://crrev.com/077c51f05cbc3dab4b0c07558012cdb132535204/components/history/core/browser/top_sites_impl_unittest.cc
,
Oct 5 2017
,
Jan 22 2018
,
Jan 23 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sky@chromium.org
, Sep 20 2017