TopSites sends out lots of fake TopSitesChanged notifications |
||||
Issue descriptionThe TopSites service keeps track of the data for the top "most visited" sites. Its update is triggered by opening new tab pages for most of the time. Ideally, the service should send out TopSitesChanged notifications to its observers only when there is a change in the top site list. However, this is not the case in the TopSites implementation. It's sending out fake TopSitesChanged notifications when clearly there is no change at all. Besides, the service constantly polls the top sites DB regardless of the presence of user activity. This results in sending out more fake TopSitesChanged notifications. The observers of TopSites are fooled by these fake notifications. As a result, they are executing much more tasks than necessary. The jumplist has been suffering from this for a long time. It receives at least 2.5 times more TopSitesChanged notifications than it should. This should be fixed!
,
Aug 31 2017
Re comment 1: Here is what I found in the code. InstantService::OnNewTabPageOpened() asks TopSites to SyncWithHistory(), which eventually calls TopSitesImpl::SetTopSites through several call stacks. This method sends out a notification no matter what. What's more, this method restarts a timer (delay time is dynamically determined) that queries history for top sites after sending out the notification. This results in sending out notifications over and over again. IMHO, sending out fake notifications is a implementation failure. Constant querying history DB to "ensure we stay in sync with history" as commented may be a design drawback. I don't think we should do polling for this.
,
Aug 31 2017
I'm not familiar with InstanceService. Seems to me SetTopSites should early out if the new/old are the same. That would at least fix part of the problem.
,
Aug 31 2017
That is definitely one fix I need to land. Thanks.
,
Sep 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e46f416012b33d4cf475fae24d945258b1981acc commit e46f416012b33d4cf475fae24d945258b1981acc Author: Xi Cheng <chengx@chromium.org> Date: Fri Sep 08 23:09:32 2017 Send out TopSitesChanged notifications only when there is a change The TopSites service keeps track of the data for the top "most visited" sites. Ideally, the service should send out TopSitesChanged notifications to its observers only when there is a change in the top site list. However, this is not the case in the TopSites implementation. It's sending out fake TopSitesChanged notifications when clearly there is no change at all. The observers of TopSites are fooled by these spurious notifications. As a result, they are doing a lot of useless work, which is a huge waste of resources. This CL fixes this issue. Now TopSites sends out update notifications only when there is a change of either urls or titles in its maintained top site list. Bug: 761063 Change-Id: Id90cd216bff585f4995ab5ad2fc08ee5b6ca77e2 Reviewed-on: https://chromium-review.googlesource.com/651608 Commit-Queue: Xi Cheng <chengx@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#500726} [modify] https://crrev.com/e46f416012b33d4cf475fae24d945258b1981acc/components/history/core/browser/top_sites_impl.cc [modify] https://crrev.com/e46f416012b33d4cf475fae24d945258b1981acc/components/history/core/browser/top_sites_impl_unittest.cc
,
Sep 9 2017
,
Jan 22 2018
,
Jan 23 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sky@chromium.org
, Aug 31 2017