New issue
Advanced search Search tips

Issue 767247 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

Investigate and remove redundant HistoryBackend::QueryMostVisitedURLs runs

Project Member Reported by chengx@chromium.org, Sep 20 2017

Issue description

The 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.

 

Comment 1 by sky@chromium.org, Sep 20 2017

I think the problem is almost any navigation many influence most visited. Hence the current code.

Comment 2 by chengx@chromium.org, 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.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by chengx@chromium.org, 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.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Comment 7 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 8 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment